-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
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
[system] Add container queries utility #41674
Conversation
@@ -1,3 +1,3 @@ | |||
export default class MuiError { | |||
constructor(message: string); | |||
constructor(message: string, ...args: string[]); |
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.
Come across this since my test is the first .ts
that uses MuiError
with multiple parameters.
Netlify deploy preview
@material-ui/core: parsed: +0.26% , gzip: +0.26% Bundle size reportDetails of bundle changes (Toolpad) |
If I understand correctly, this adds functionality but doesn't modify the existing media query one, right? I mean if you were doing { xs: { ... } } You'll still be able to do it, and with this change you can add custom queries { xs: { ... }, '@350': {...} } So isn't it opt-in by design? Is there something else you refer to with "should we add container queries to the default theme"? |
The changes I made right now are kinda mixed between the two so let's neglect the code for now. We need to decide between
|
Are there any breaking changes associated with the new shorthand?
Regarding browser support, it's important to consider it when users cannot opt out of features. For example, if we use the CSS property |
As a user, I'd prefer it to be the default, so I don't have to |
Yes, that's correct. It's the responsibility of developers to check browser support before using it.
Because there will be computation overhead (the impact is based on the project size) for those who don't want to use container queries. This is the only concern I have. Anyway, I don't mind making it as a default too. |
…/container-query
…/container-query
@DiegoAndai @mnajdova Would you mind taking a look at this PR again? |
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.
Looks good overall, just have a couple of minor questions
@@ -9,6 +9,7 @@ describe('defaultTheme', () => { | |||
'colorSchemeSelector', | |||
'defaultColorScheme', | |||
'breakpoints', | |||
'cq', |
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.
Could we (or should we) use containerQueries
/containerQuery
instead of cq
?
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.
Sure, it can be explicit on this.
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.
Let me know if you decide to implement this change or not 😊
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.
Done! changed to containerQueries
(with ies)
} | ||
|
||
/** | ||
* For using in `sx` prop to sort the breakpoint from low to high. |
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.
Is this function only used for the xs
, sm
, md
, lg
, and xl
queries?
I'm wondering as it only supports min-width
declarations.
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, it supports valid CSS units like 40em
or 300px
but it requires all fields to use the same unit.
sx={{
flexDirection: {
'@300px': 'column',
'@500px': 'row',
}
}}
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.
But it only supports min-width
ones, right? So, for example, theme.cq.down
, which outputs @container (max-width:...)
won't be sorted correctly.
I want to better understand this function's use case. Is it internal or also for users?
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.
The sorting only happens for the sx
prop (happens internally, the function isn't meant for users) similar to theme breakpoints shorthand.
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.
Nice work @siriwatknp 🚀
closes #36670
Context
The reasons I spend time on this:
✅ Need decision
I decided to make it a default theme. cc @DiegoAndai
Should we add container queries to the default theme or should it be opt-in?