Repository navigation
feat(gallery): resolve breakpoints from config - #31519
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
09e5ae2 to
c9ad644
Compare
ShaneK
left a comment
ShaneK
left a comment
There was a problem hiding this comment.
Looks good to me! The only real issue is a gallery narrower than a configured xs above 0 ending up with no column count, which has a one-line fix. The rest are just nits.
| providedValue === undefined || sanitizedProvided === undefined ? sanitizedDefault : sanitizedProvided; | ||
|
|
||
| if (resolved !== undefined && width >= BREAKPOINTS[bp]) { | ||
| if (resolved !== undefined && width >= breakpoints[bp]) { |
There was a problem hiding this comment.
| if (resolved !== undefined && width >= breakpoints[bp]) { | |
| if (resolved !== undefined && (bp === 'xs' || width >= breakpoints[bp])) { |
Now that xs comes from config it can be above 0, and a gallery narrower than it won't match anything here. The DEFAULT_COLUMNS fallback runs through this same loop, so we end up setting --internal-gallery-columns to "undefined", which the 2 fallback in the stylesheet can't catch since the variable is set. With screenBreakpoints: { xs: 400 } and a 300px gallery, it collapses to one column with every item on its own row, and masonry skips layout.
Since the gallery measures its own width, I think one in a card or sidebar could hit this pretty easily. Letting xs always match keeps the behavior from when it was hardcoded to 0, and with it that case resolves to the xs value while the existing specs and breakpoints e2e still pass. Could you add a spec case for it too, something like xs: 400 at a width of 300?
There was a problem hiding this comment.
I can update it but this seems weird no? Shouldn't one breakpoint always be 0? Do we add an xxs that must be 0, or require xs to be 0? Should I make a follow-up for this?
Fix: 085bb4b
There was a problem hiding this comment.
I'm not sure, this is just a weird edge case that could come up I noticed. I think it's extremely unlikely, but 🤷♂️
There was a problem hiding this comment.
The default of 0 is enough, I don't think we need a follow-up. If an app moves xs above 0, what happens below it is on the app. A quick check on how other frameworks handle a raised smallest breakpoint could be good to review, though, in case there's a convention worth matching.
There was a problem hiding this comment.
I decided to remove the xs breakpoint from the screenBreakpoints config. With the way the new CSS is written, there would be no way to target the screen size under the xs breakpoint if it was larger than 0. Additionally, adding this code above just made it so that them setting it to anything above 0 was ignored for smaller breakpoints anyway: 89fbc37
c9ad644 to
21b785c
Compare
thetaPC
left a comment
thetaPC
left a comment
There was a problem hiding this comment.
LGTM, as long as we can address FW-7285 soon
| providedValue === undefined || sanitizedProvided === undefined ? sanitizedDefault : sanitizedProvided; | ||
|
|
||
| if (resolved !== undefined && width >= BREAKPOINTS[bp]) { | ||
| if (resolved !== undefined && width >= breakpoints[bp]) { |
There was a problem hiding this comment.
The default of 0 is enough, I don't think we need a follow-up. If an app moves xs above 0, what happens below it is on the app. A quick check on how other frameworks handle a raised smallest breakpoint could be good to review, though, in case there's a convention worth matching.
b026453 to
f7d11b5
Compare
f7d11b5 to
28d3d08
Compare
cfe3ecf to
ddd2b29
Compare
ddd2b29 to
1bfb373
Compare
The gallery now uses the shared screen breakpoints and respects the `screenBreakpoints` config. Breakpoints are still matched against the gallery's own width rather than the width of the screen, which is now noted in the properties. --------- Co-authored-by: Brandy Smith <6577830+brandyscarney@users.noreply.github.com>
This branch was successfully deployed
3 participants
The gallery now uses the shared screen breakpoints and respects the
screenBreakpointsconfig. Breakpoints are still matched against the gallery's own width rather than the width of the screen, which is now noted in the properties.