Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade MaterialUI to v4 and provide MaterialUI via CDN #101

Closed
1 task done
louise-davies opened this issue Jun 21, 2019 · 8 comments · Fixed by #132
Closed
1 task done

Upgrade MaterialUI to v4 and provide MaterialUI via CDN #101

louise-davies opened this issue Jun 21, 2019 · 8 comments · Fixed by #132
Assignees
Labels
enhancement New feature or request not MVP not part of the minimum viable product

Comments

@louise-davies
Copy link
Member

louise-davies commented Jun 21, 2019

Description:
New updates in MaterialUI v4 means that the library can be more easily provided via CDN, meaning ADR 7 can be revisited. However, this requires an upgrade to MaterialUI v4 and there are some breaking changes that will need to be refactored.

Note: do we even want to do this? doing it this way means we are providing the whole library once, whereas making each plugin import it's own means we only pick the things we need but consequently we import those things multiple times. We might need to investigate which results in less code being downloaded to the client

Acceptance criteria:

  • Upgraded material-ui dependency to v4
  • [ ] Investigate whether supplying material-ui as an external dependency actually improves efficiency or whether tree-shaking is more efficient - probably depends on the amount of plugins being loaded.
@louise-davies louise-davies added enhancement New feature or request not MVP not part of the minimum viable product labels Jun 21, 2019
@louise-davies louise-davies self-assigned this Jun 21, 2019
@louise-davies
Copy link
Member Author

After upgrading to material ui v4, we should use the new StylesProvider to provide a generateClassName so we can prefix our styles with an app specific prefix (e.g. sgw)

@agbeltran agbeltran assigned GoelBiju and unassigned louise-davies Dec 12, 2019
@GoelBiju
Copy link
Contributor

GoelBiju commented Dec 18, 2019

Just for additional information, material-ui have a migration page here dedicated to moving from v3 to v4. There is a supporting GitHub README.

A material-ui guide to minimising bundle size is available as well.

@GoelBiju
Copy link
Contributor

GoelBiju commented Jan 17, 2020

Specific material-ui v4 changes:

  • theme.spacing.unit has been deprecated and now replaced with the new API: theme.spacing() which allows for number parameters,

  • Grid container spacing prop has changed to only accept values between 0 - 10 in order to stop mentally counting in 8's, so a spacing of 24 now becomes 3 in material-ui v4,

  • ListItem button prop typing - the composition of the LinkListItem needs to be changed (this may be addressed by a solution in this composition guide),

  • Remove useNextVariants for typography in theme creation,

  • withTheme has changed so that we can remove the first option argument and can just have the component as a single argument.

@GoelBiju
Copy link
Contributor

GoelBiju commented Jan 17, 2020

As a result of the upgrade, unit tests are failing due to the ukri object (within the theme - UKRITheme) not being recognised. The shallow rendering is not working with the custom theme as it is not being passed down. mount works however we do not want to use this with the unit tests.

@GoelBiju
Copy link
Contributor

GoelBiju commented Jan 19, 2020

As a result of the upgrade, unit tests are failing due to the ukri object (within the theme - UKRITheme) not being recognised. The shallow rendering is not working with the custom theme as it is not being passed down. mount works however we do not want to use this with the unit tests.

@louise-davies has suggested a way of constructing the wrapper to hold the components without any styles/redux so that we can manually provide the props. The unit tests need to now be updated to complete the transition to material-ui v4.

  • Fix SciGateway unit tests by testing components without passing styling/redux information.

@GoelBiju
Copy link
Contributor

Since the second objective to invesigate whether providing material-ui as an external dependency or to use it through tree-shaking requires completed plugins/packages, I will keep it for now and create a draft pull request to address the version 4 update for material-ui @louise-davies .

@GoelBiju
Copy link
Contributor

@louise-davies @agbeltran I have removed the task of investigating the material-ui bundle size and CDN/tree-shaking methods to a separate issue (#137 ). This issue can be closed once the pull request for the MaterialUI v4 upgrade (#132 ) has been merged into master.

@agbeltran
Copy link
Member

thank you @GoelBiju !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request not MVP not part of the minimum viable product
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants