-
Notifications
You must be signed in to change notification settings - Fork 31
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
MUI Grid Cleanup #2567
base: main
Are you sure you want to change the base?
MUI Grid Cleanup #2567
Conversation
8018960
to
38497c5
Compare
Quality Gate passedIssues Measures |
Har du tatt i bruk designsystemet sine tokens her? *Edit: Dette gjøres senere |
38497c5
to
38e755c
Compare
881d0bd
to
e44e282
Compare
# Conflicts: # src/layout/GenericComponent.tsx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solid piece of work! 🥳 Great job! 👏 🚀 The only thing I'm hesitant about is the breakpoints (see relevant comment), but otherwise this looks very good.
/* Small (sm breakpoint) */ | ||
@media (min-width: 600px) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These breakpoints don't match what we've been using before (see the exported breakpoints
from useDeviceWidths.ts
). If you took the default values from MUI, remember that we're overriding the theme (see altinnApTheme.tsx
, which also don't match the breakpoints in useDeviceWidths.ts
😕)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had the correct breakpoints at first, but i got a diff from percy, and i changed it all back. I will look deeper into it! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see now that we need the current breakpoints as it is, because this corresponds with the MUI Grid component. They are equal to the overriding theme values(altinnApTheme.tsx
). I think that we should consider the changing the values of the spacing/breakpoints/etc to correspond with the designsystemet as a next iteration on the Flex component. Now the focus is to get rid of MUI without breaking changes.
size={{ | ||
xs: displayGrid?.xs ?? 12, | ||
sm: displayGrid?.sm, | ||
md: displayGrid?.md, | ||
lg: displayGrid?.lg, | ||
xl: displayGrid?.xl, | ||
}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't these do the same thing? 🤔
size={{ | |
xs: displayGrid?.xs ?? 12, | |
sm: displayGrid?.sm, | |
md: displayGrid?.md, | |
lg: displayGrid?.lg, | |
xl: displayGrid?.xl, | |
}} | |
size={displayGrid} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the difference is that the fallback for xs on 12 is not applied.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if you don't specify a size for xs
, what does it do when you shrink it down? 🤔 I would imagine 12 is a good default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you dont specify size for xs you dont get fluid sizing. This is the default behavior of MUI. it would still be 12 as a default but not fluid sizing. We could rewrite this to be default fluid 12, but it would require a rewrite css of many of our components. The priority in this PR is just to replicate what we already had. Many possibilities to improve our grid system now that we own it ourselves.
Co-authored-by: Ole Martin Handeland <[email protected]>
Co-authored-by: Ole Martin Handeland <[email protected]>
…re/mui-grid-cleanup
Quality Gate passedIssues Measures |
Description
Remove use of MUI Grid component and replace it with our own Flex component, as a part of the way to get rid of the dependency to material-ui.
Related Issue(s)
Verification/QA
kind/*
label to this PR for proper release notes grouping