chore(Storybook): Keep the as prop off the Chromatic variant matrix - #2906
Open
VincentSmedinga wants to merge 3 commits into
Open
chore(Storybook): Keep the as prop off the Chromatic variant matrix#2906VincentSmedinga wants to merge 3 commits into
VincentSmedinga wants to merge 3 commits into
Conversation
Swapping a component's tag never changes how it looks, so every non-default tag rendered a row identical to the baseline. Generalise the aria-only exclusion to cover every prop that cannot change appearance. The Grid test story drops from 22 rows to 13, Row from 18 to 16 and Column from 16 to 14.
Contributor
Coverage Report for React components
File CoverageNo changed files found. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the Storybook Chromatic “variant matrix” generation to exclude the as prop from the prop-axis, because varying as for these layout components produces visually identical snapshots and adds substantial redundant snapshot height.
Changes:
- Rename
ARIA_ONLY_PROP_NAMES→NON_VISUAL_PROP_NAMESand includeasin that exclusion list inbuildVariantMatrix. - Add a unit test ensuring
asdoes not create rows on the prop axis. - Update Storybook documentation to state that non-visual props (including
as) are excluded from the matrix.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| storybook/src/_common/buildVariantMatrix.ts | Excludes as (alongside accessible-name-only props) from the prop axis to avoid redundant visual variants. |
| storybook/src/_common/buildVariantMatrix.test.ts | Adds coverage verifying as does not contribute prop-axis entries. |
| documentation/storybook.md | Documents that non-visual props (accessible name props and as) are omitted from the matrix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
|
Size Change: 0 B Total Size: 420 kB ℹ️ View Unchanged
|
…rop-from-variant-matrix # Conflicts: # storybook/src/_common/buildVariantMatrix.ts
Contributor
Author
|
Probably best to merge this after our Chromatic plan has room again – or else these changes will surprisingly appear in later, unrelated regression tests. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe the pull request
Links
The test stories carry the
!devtag, so they are not in the sidebar of the deploy; the links above open them directly.What
The
asprop no longer gets a row of its own in the variant matrix that the Test stories render for Chromatic. Three test stories shrink as a result: Grid from 22 rows to 13, Row from 18 to 16, and Column from 16 to 14.Why
buildVariantMatrixputs every prop the argTypes offer on the prop axis and renders one row per non-default value. Forasthat means one row per allowed tag, and every one of those rows is pixel-identical to the baseline, because swapping the element a component renders does not change how it looks.Grid is where this cost the most. Its ten tags produced ten rows sharing one appearance, 4878px of a 9117px story, so a fifth of what Chromatic snapshotted for Grid was a copy of the row above it.
How
buildVariantMatrixalready had this idea.ARIA_ONLY_PROP_NAMESexisted because varying an accessible name also produces a cell identical to the baseline.asbelongs to the same category, so rather than add a second list beside it, the constant is nowNON_VISUAL_PROP_NAMESand covers both. Its comment carries the reason, which the name alone does not give.olandulare the one case where a tag genuinely could change the appearance, since a browser indents a list and draws markers. They render like adivonly because.ams-grid:is(ol),.ams-grid:is(ul)and the matching Subgrid rules reset them, and because.ams-grid__cell:is(li)takesdisplay: blockfor a Safari marker bug. That reset is covered by theListCaseandGridListCaseblocks of the Grid test story, which put a list Grid and Subgrid beside identical plain ones. Those blocks are untouched, and they are what makes the matrix rows redundant rather than the other way round. OnlygridTagscontainsolandul;rowTags,columnTags,spotlightTagsandbreakoutCellTagsarearticle,divandsectiononly.Merging #2907 put two lists next to each other, so each comment now says which one to reach for.
NON_VISUAL_PROP_NAMESholds props that cannot change the appearance at all;UNVARIED_PROP_NAMESholds props whose values do look different and are snapshotted somewhere else, assvgis by Icon.asbelongs to the first: leaving it out is not a decision about coverage, there is simply nothing for a snapshot to catch.That merge also matters for the guard #2907 added.
asreachesbuildVariantMatrixwith options, sinceasArgTypegives it a control, so it is exactly the kind of prop that guard throws for once its type fails to resolve. Excluding it from the axis puts it out of that check's reach, which is correct because leaving it unvaried is deliberate. Two of the guard's tests moved with it: the one that showed a message naming more than one prop usedasas an example and now usesgapVertical, and a new test coversasoffering options without throwing.Eight components take an
asprop, but only three test stories change. Grid Cell, Grid Subgrid and Breakout Cell have no test story of their own, and the Spotlight and Breakout test stories write out one composition by hand instead of callingrenderComponentVariants, as the comment in the Breakout story explains.Verified in Chromium at 1440px by counting the children of the variants wrapper before and after: 22 to 13, 18 to 16, and 16 to 14, with every remaining row distinct. The Grid story is 7389px tall where it was 9117px.
Checklist
Before submitting your pull request, please ensure you have done the following. Check each checkmark if you have done so or if it wasn't necessary:
/chromatic testand verify visual regression tests passAdditional notes
Expect a Chromatic diff on the Grid, Row and Column test stories, and nothing else. All of it is deletions: the removed rows are the tag variants, and every row that remains is unchanged.
documentation/storybook.mdgained a sentence, since its description of what the matrix leaves out was the place a reader would look for this rule.