-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
core: add 0 case to responsive props #101440
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
Conversation
2cf9ae4 to
99678a6
Compare
|
|
||
| const BREAKPOINT_ORDER: readonly Breakpoint[] = ['xs', 'sm', 'md', 'lg', 'xl']; | ||
| function getBreakpoint(breakpoint: Breakpoint, theme: Theme) { | ||
| if (breakpoint === '0') { |
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.
l: not sure if '0' is intuitive. IMO default or base are more descriptive
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.
What would you expect the behavior of display={{default: 'none', xs: 'block'}} to be?
In the current case with a 0, it means
0 -> xs = none, then block.
But a default might be misunderstood in the sense that it applies to all unspecified breakpoints, which it does not, and I don't think it should
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 a default might be misunderstood in the sense that it applies to all unspecified breakpoints, which it does not, and I don't think it should
Valid point.
After a second thought, when I first used it I actually thought xs is 0-500, since I thought it is following the mobile first principle. So I'm also not sure if 0 would solve it entirely.
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 think the biggest confusion might come from the fact that we’re trying to express ranges with a single value. Usually, xs has a single value, e.g. on gap=“xs” we know that it is a certain amount of px.
But on responsive props, xs actually means “xs and above”, or, if we have “md” also defined, it’s “from xs to md”.
I also couldn’t find good documentation on this topic; there is a bit on Container and an example on Stack but that’s about it. I think we should have a dedicated page about it like we have for “Layout Composition”.
That said, I also find 0 adds more confusion (naming wise). An explicit naming would probably be ”<xs”, as in, “smaller than xs”, but I’m not sure if we should go that route 😅.
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 on responsive props, xs actually means “xs and above”, or, if we have “md” also defined, it’s “from xs to md”.
Yeah, this has been a tradeoff in allowing the min breakpoints to be specified as opposed to having to specify all. Regardless, we would have always probably had this issue in one way or another where we would need to capture > max or < min.
I also couldn’t find good documentation on this topic; there is a bit on Container and an example on Stack but that’s about it. I think we should have a dedicated page about it like we have for “Layout Composition”.
Yes, I can add that.
That said, I also find 0 adds more confusion (naming wise). An explicit naming would probably be ”<xs”, as in, “smaller than xs”, but I’m not sure if we should go that route 😅.
An alternative could also be to define a 2xs breakpoint that does the same thing as what 0 does now, which might be better than 0. I don't like using < operator as that might create confusion if we use {{'<xs'..., 'md': ...}} which would effectively cause <xs to span from 0 to md
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.
2xs sounds great
|
Closing in favor of an alternative approach |
Add a way to enable styling of < xs breakpoint sizes by introducing a 0 breakpoint that spans from 0 screen size to the first defined breakpoint