Repository navigation
Grid: accept the column count as steps by container width - #85216
Conversation
columns and minColumnWidth compose; one Columns story replaces Responsive and Layered
columns accepts a count or a map by container width
the floor alone decides the count from that width up
eight tiles with explicit width and height
🤖 PR meta 🤖🎉 PropsIf you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. Updated as activity occurs, without notifying anyone named here. Add the 📦 Bundle sizeSize Change: +24 B (0%) Total Size: 9.05 MB 📦 View Changed
⚡ PerformanceShow the resultsClient side metrics exclude the server response time. front-end-block-theme
front-end-classic-theme
media-processing
media-upload
post-editor
site-editor
🏁 Flaky testsSome tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information. Regression: updating a specific option will only change its value and will not corrupt others in
|
| const cap = resolveColumnCap( policy.columns, containerWidth ); | ||
| const { minColumnWidth } = policy; | ||
| if ( ! minColumnWidth || minColumnWidth <= 0 ) { | ||
| return Math.max( 1, cap ?? defaultColumns ); |
There was a problem hiding this comment.
Could we keep the previous step's count for an 'auto' step when minColumnWidth isn't set? Right now it falls back to six, so { 0: 8, 960: 'auto' } drops from 8 to 6 columns at 960px. The dashboard doesn't pass minColumnWidth today, so it would hit this with steps.
There was a problem hiding this comment.
+1. Two additions: the dashboard's own default is 4 (WIDGET_DASHBOARD_COLUMN_COUNT), so with the current fallback an 'auto' step there would render the grid's 6, not even the dashboard's default. And the renders the default on an auto step with no floor test locks in the 6, so it would need to change along with this.
There was a problem hiding this comment.
it does make sense. Thanks for the feedback. Done in b0038dc
With no floor, an 'auto' step is skipped, and the count below it holds, so { 0: 8, 960: 'auto' } renders 8 at every width. { 0: 'auto' } alone still takes the surface default, so that test stays, with a new row for the count below. The README and the ColumnSteps doc say so.
| } | ||
| const reached = widths.find( ( width ) => containerWidth >= width ); | ||
| const step = columns[ reached ?? widths[ widths.length - 1 ] ]; | ||
| return typeof step === 'number' ? step : undefined; |
There was a problem hiding this comment.
Would it be worth normalizing step values here, the way resolveDashboardColumnCap does? I noticed a string like '3' lifts the cap like 'auto', and 2.5 or NaN reach grid-template-columns as is. Since the follow-up replaces that helper, its rounding and finite check would otherwise be lost.
There was a problem hiding this comment.
+1. On DashboardLanes, a NaN step goes further than a bad grid-template-columns: Math.max( 1, NaN ) stays NaN, and new Array( NaN ) in lane-placement.ts throws a RangeError. A plain columns={ NaN } does the same on trunk, so normalizing in resolveColumnCount for both the number and the steps would cover both.
There was a problem hiding this comment.
👍
Done in 4edad78, for the number and the map alike: counts are rounded down and never below one; a count that is not a finite number is unset; and a step that is neither a count nor 'auto' is skipped, so the count below it holds.
The floor also needs a finite positive value. Lanes no longer see a NaN. The README states the rule, and the changelog entry carries it.
| `columns` and `minColumnWidth` compose. Both surfaces measure their | ||
| own width with a `ResizeObserver` and resolve the count from it. |
There was a problem hiding this comment.
nit: Could we keep each new paragraph on one line? AGENTS.md asks for soft-wrapped Markdown, even when an older file like this one is hard-wrapped. Same for the other new paragraphs in this section.
| `columns` and `minColumnWidth` compose. Both surfaces measure their | |
| own width with a `ResizeObserver` and resolve the count from it. | |
| `columns` and `minColumnWidth` compose. Both surfaces measure their own width with a `ResizeObserver` and resolve the count from it. |
| DashboardLanesLayoutItem, | ||
| DashboardLanesProps, | ||
| } from './dashboard-lanes/types'; | ||
| export type { ColumnPolicy, ColumnSteps } from './shared/column-policy'; |
There was a problem hiding this comment.
Is there a consumer in mind for ColumnPolicy? It only types the parameter of the internal resolveColumnCount, so exporting just ColumnSteps might be enough for now. Adding it back later wouldn't be breaking.
| export type { ColumnPolicy, ColumnSteps } from './shared/column-policy'; | |
| export type { ColumnSteps } from './shared/column-policy'; |
| if ( columns === undefined || typeof columns === 'number' ) { | ||
| return columns; | ||
| } | ||
| const widths = Object.keys( columns ) |
There was a problem hiding this comment.
columns={ null } from untyped JS used to fall back to the default through columns ?? DEFAULT_COLUMNS. Now typeof null is 'object', so it gets past the undefined and number check and throws on Object.keys( null ). It's outside the TypeScript contract, so it's low priority, but checking columns == null on line 31 would keep the old behaviour.
| return undefined; | ||
| } | ||
| const reached = widths.find( ( width ) => containerWidth >= width ); | ||
| const step = columns[ reached ?? widths[ widths.length - 1 ] ]; |
There was a problem hiding this comment.
When the container is narrower than the smallest key, the smallest step's count applies, so { 600: 2, 960: 4 } gives 2 columns at 300px. That seems a sensible fallback, and the tests cover it, but neither the PR description nor the README says so. The README describes each count as applying "from each container width up", which reads as if nothing applies below the first key. Could the Columns section mention it, or recommend starting the map at 0?
There was a problem hiding this comment.
Thanks, Lourens. It's documented in c8db0ab, right after the map example in the README and in the ColumnSteps doc
What?
columnsonDashboardGridandDashboardLanesaccepts aColumnStepsmap, besides a number.{ 0: 1, 600: 2, 960: 4 }renders one column from0px, two from600pxand four from960px.A step can be
'auto'. From that width up there is no cap, andminColumnWidthalone decides the count.A number resolves exactly as before.
minColumnWidthcomposes with every shape.Why?
A surface takes one count for every width. Stepping it by container width has to live in the consumer, and
@wordpress/widget-dashboardhardcodes960pxand600px.A host should declare that policy as data, next to the floor, and let the surface apply it from its own measured width and gap.
The
'auto'step serves hosts that want fixed counts on narrow containers and a floor-driven count above them.The dashboard package forwards the policy in a follow-up PR.
How?
packages/grid/src/shared/column-policy.ts. New.resolveColumnCapreturns the policy's count at a container width, orundefinedwith no count or an'auto'step.resolveColumnCountlowers it by the floor, fits the floor alone when no count applies, and falls back to the default, never below one.packages/grid/src/dashboard-grid/index.tsxandpackages/grid/src/dashboard-lanes/index.tsx. Both surfaces callresolveColumnCountinstead of their inline math, with the same inputs: measured width, computed gap, default of six.packages/grid/src/dashboard-grid/types.ts,packages/grid/src/dashboard-lanes/types.tsandpackages/grid/src/index.ts.columns?: number | ColumnSteps, the prop docs, and theColumnPolicyandColumnStepsexports.packages/grid/src/shared/test/column-policy.test.ts. Unit rows for both functions: pass-through, the step at each width, the'auto'step, floor alone, cap over floor, a step lowered by the floor, never below one.Stories.
ResponsiveandLayeredmerge into oneColumnsstory per surface.ColumnStepsshows the map;ColumnStepsWithFloorshows the'auto'step over a floor.packages/grid/README.mdandpackages/grid/CHANGELOG.md. The Columns section describes the three shapes with one example each. One enhancement entry.Testing Instructions
npm run storybook:dev, open Widgets → Grid → DashboardGrid → Column Steps. Resize the window: one column under600px, two up to960px, four from960px.960px; from960pxthe240pxfloor decides: four columns from1032px, five from1296px.columnsto4andminColumnWidthto240. Unchanged from trunk: the count drops one at a time as a column would fall under240px.200px.npm run test:unit -- packages/grid/src/shared/test/column-policy.test.ts: 13 tests pass.Testing Instructions for Keyboard
No new interface. In the
Columnsstories, enableeditModeand move or resize a tile with the keyboard. Gestures snap to the resolved count under every shape ofcolumns.Screenshots or screencast
Resolved count by container width, default
24pxgap:columnsminColumnWidth4{ 0: 1, 600: 2, 960: 4 }{ 0: 1, 600: 2, 960: 4 }300{ 0: 1, 600: 2, 960: 'auto' }240DashboardGrid, Column Steps With Floor
[video: DashboardGrid, Column Steps With Floor]
DashboardLanes, Column Steps With Floor
[video: DashboardLanes, Column Steps With Floor]