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

Migrated to MUI v5 #664

Closed
wants to merge 2 commits into from
Closed

Conversation

awanlin
Copy link
Collaborator

@awanlin awanlin commented Oct 9, 2024

This migrates the Demo site App to use MUI v5 but there are still dependencies that use Material-UI v4 like @backstage/core-components.

Note: this migration was pretty easy the place where I ran into the most issues was hitting and "theme.spacing is not a function" TypeError. The fix was making sure that the @mui/material and @mui/styles versions matched what we were using upstream. This comment on this issue was key to helping me solve this: mui/material-ui#30611 (comment)

"@material-ui/icons": "^4.9.1",
"@mui/icons-material": "5.14.18",
"@mui/material": "5.14.18",
"@mui/styles": "5.14.18",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there's a better way to keep this in sync with Backstage itself I'm very much open to ideas.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@webark curious if you have any suggestions here based on you experience?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We set a resolution in the root.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be the case that we could revisit these being peer dependencies, like we do for react, react-dom etc. I.e. that all Backstage packages put them as peer deps and then it's up to the adopter to fulfil those in their packages/app to a concrete version. But I'm not sure it's the perfect solution, would have to bring it up with the team

Signed-off-by: Andre Wanlin <awanlin@spotify.com>
Signed-off-by: Andre Wanlin <awanlin@spotify.com>
"@material-ui/icons": "^4.9.1",
"@mui/icons-material": "5.14.18",
"@mui/material": "5.14.18",
"@mui/styles": "5.14.18",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be the case that we could revisit these being peer dependencies, like we do for react, react-dom etc. I.e. that all Backstage packages put them as peer deps and then it's up to the adopter to fulfil those in their packages/app to a concrete version. But I'm not sure it's the perfect solution, would have to bring it up with the team

@@ -166,6 +166,7 @@ export const apertureTheme = createUnifiedTheme({
},
},
},
// @ts-ignore
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm just for this one?

@awanlin
Copy link
Collaborator Author

awanlin commented Nov 13, 2024

Closing for the time being based on some aspects still needing refinement.

@awanlin awanlin closed this Nov 13, 2024
@awanlin awanlin deleted the topic/mui5-migration branch November 13, 2024 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants