feat(grid, col): resolve breakpoints from config - #31518
brandyscarney wants to merge 10 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
ShaneK
left a comment
There was a problem hiding this comment.
Looks really good overall! Just a couple of things, mainly the grid and col holding onto a stale breakpoint after being removed and re-added, plus the deprecation warning firing once per column. The rest are small.
| @Prop() fixed = false; | ||
|
|
||
| connectedCallback() { | ||
| this.unsubscribeBreakpoint = onBreakpointChange(() => forceUpdate(this)); |
There was a problem hiding this comment.
| this.unsubscribeBreakpoint = onBreakpointChange(() => forceUpdate(this)); | |
| this.unsubscribeBreakpoint = onBreakpointChange(() => forceUpdate(this)); | |
| forceUpdate(this); |
If a grid is removed and re-added after the screen crosses a breakpoint, it keeps the old screen-breakpoint, since reconnecting only re-subscribes. A fixed grid removed at 800px and re-added at 1300px stays 720px wide instead of 1140px, and a size-md="6" col taken from 1000px down to 600px the same way keeps its half width.
Before this the padding and widths came straight from media queries, so they couldn't go stale, and now nothing corrects them until the next breakpoint crossing. I think that could bite things like Vue's KeepAlive or Angular's detached views. Forcing an update after subscribing fixes both cases and the grid tests still pass with it. The col would need the same change.
| // TODO(FW-7557): Remove these in v11. | ||
| // Keep track of which deprecation warnings have been printed so they are | ||
| // not repeated on every re-render or screen resize. | ||
| private hasWarnedDeprecatedProps = false; |
There was a problem hiding this comment.
Since these flags are per instance, every column with a suffixed prop logs its own warning, so a grid with five size-md columns prints five of them. Most existing apps use the suffixed props, so I think anything with a few dozen columns is going to flood the console. Moving both flags to module scope gets it down to one per page, though the "warns that they are deprecated" spec would then need a way to reset them between tests.
Separately, was the plain HTML case considered? Once these are removed in v11 there's no way to set responsive sizes in markup, since the object form only works as a property.
There was a problem hiding this comment.
Added a separate file for the deprecation warnings & now print the actual code that should be written: f4742cc
Separately, was the plain HTML case considered? Once these are removed in v11 there's no way to set responsive sizes in markup, since the object form only works as a property.
Honestly, no. I was just thinking that JS was required. Created FW-7822 to look into supporting this.
50bec8b to
f561714
Compare
Issue number: internal
What is the current behavior?
The previous PR in this stack added the
screenBreakpointsconfig option and the@utils/breakpointshelpers, but nothing consumes them yet.ion-gridgets its responsive padding and fixed widths purely from@mediaqueries, with the thresholds compiled into the CSS.ion-colonly expresses per-breakpoint values through breakpoint-suffixed properties (size-xsthroughsize-xl, plus theorder-*andoffset-*equivalents). Neither reads the configured breakpoints, and neither can targetxxl.ion-gridalso declared its ownION_GRID_BREAKPOINTS/IonGridBreakpoint(carrying aTODO(FW-7285)) rather than using the shared type.What is the new behavior?
ion-gridresolves the active breakpoint in JavaScript and reflects it on the host as ascreen-breakpointattribute. It subscribes viaonBreakpointChangeinconnectedCallbackand unsubscribes indisconnectedCallback, so it re-renders when the screen crosses a configured threshold.@mediacopy scoped to:host(:not([screen-breakpoint])), and an attribute copy:host([screen-breakpoint="md"]). The attribute copy is what the config drives; the@mediacopy is the baseline before the component hydrates and when JavaScript never runs. This split is necessary because CSS media queries cannot read custom properties, so avar()cannot be used to change a breakpoint.ion-colaccepts an object of breakpoint values forsize,orderandoffset:col.size = { xs: 12, md: 6 }. This is resolved against the configured breakpoints.xxlis added to this object. An empty string ornullat a breakpoint resets that column to the default flex layout.ION_GRID_BREAKPOINTS/IonGridBreakpointare replaced by the sharedScreenBreakpoint.Does this introduce a breaking change?
The
size,order, andoffsetprop types have been widened fromstring | undefinedtoBreakpointMap<string | number | null> | number | string | undefined. The suffixed properties continue to work as before.BREAKING.mdhas been updated under 10.x with migration guidance for these deprecations. Thepush/pullentry has also been moved from the Grid section to Col, where the properties are defined.Other information
Preview