Repository navigation
Storybook sidebar: scattered component groups, and a guideline rule that only held by accident - #1256
Conversation
…s, for every component storySort.order could only do this by naming each component, so the six listed got it and everyone else — Accordion, and anything added since — landed its guideline after its stories, with nothing to signal the omission. A comparator ranks the sections and categories as before, then puts a component's guideline first whatever the component.
Hello jeanmarcmilletscality,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| : depth === 1 && left[0] === 'Components' | ||
| ? CATEGORIES | ||
| : []; | ||
| return rankIn(order, left[depth] ?? '') - rankIn(order, right[depth] ?? ''); |
There was a problem hiding this comment.
This comparator is not transitive. Two different components tie (0), but two pages of the same component do not. For example, Checkbox/Default ties with Toggle/Guideline, and Toggle/Guideline ties with Checkbox/Guideline, yet Checkbox/Guideline < Checkbox/Default. With an inconsistent comparator, the spec leaves the sort order implementation-defined.
This matters here because the guideline docs entries are indexed after all CSF entries. V8's TimSort places them with binary insertion and galloping, so a guideline can land after its own stories when the probe hits another component's entry. Accordion working in a manual check does not prove other components work.
Break ties between unranked siblings by name so the order is total. Paths are stories/<Name>/, so this stays close to the current load order:
| return rankIn(order, left[depth] ?? '') - rankIn(order, right[depth] ?? ''); | |
| return ( | |
| rankIn(order, left[depth] ?? '') - rankIn(order, right[depth] ?? '') || | |
| (left[depth] ?? '').localeCompare(right[depth] ?? '') | |
| ); |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: |
JeanMarcMilletScality
changed the title
…relying on filenames The comparator returned 0 for any two entries with different titles, so the sort could not move a guideline past a run it had been told was equal: a page whose file does not sit beside its component's stories stayed last, with nothing to signal it. Comparing unranked names makes the order total, which also groups a component's pages together wherever their files live. stories/Icon.mdx followed the contributing recipe literally and so declared no name, which made it an unnamed attached page rather than a guideline. The recipe now carries the name, and a test fails on any attached page that omits it.
🔴 Request changesWhy the tests need changes
Findings
This review is advisory and does not block the merge. Full reviewThe test depends on real time, randomness, network or filesystem
Findings
|
JeanMarcMilletScality
changed the title
The test exercised three of the seven names, and not 'Progress & loading'. A misspelling there goes unranked rather than erroring, which drops the category below the ones that still rank — the defect this sort was written to fix, and every assertion would have stayed green.
The attached-page check matched the Meta tag as one string anchored on `of=`, so it skipped every page writing `name` first — 11 pages were checked where 18 exist. The tag is now read as a whole. The new case prints rather than asserts: most story files predate the rule, and failing the suite for them would punish work nobody has been asked to do.
…eakage The missing-guideline scan ran inside an it() with no expect, so it could not fail whatever the traversal produced, while occupying a slot a reader expects to enforce something. It now runs at module level and still prints the backlog. The comparator was sliced out of preview.tsx by matching braces from the first one after `storySort:`, which a brace inside a string or a comment would have broken. Babel locates the node instead, and the two constraints that take the whole sidebar down — the arrow must stay inline, and its parameters cannot be annotated — are now checked there, each with a message saying what to fix.
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: |
|
/approve |
Queue out of orderThe changeset has received all authorizations to enter the merge queue, Look into https://devdocs.scality.net/support/faq/#bert-e for guidance about The following options are set: approve |
|
/status |
Status
The following options are set: approve |
Queue out of orderThe changeset has received all authorizations to enter the merge queue, Look into https://devdocs.scality.net/support/faq/#bert-e for guidance about The following options are set: approve |
|
/status |
Status
The following options are set: approve |
In the queueThe changeset has received all authorizations and has been added to the The changeset will be merged in:
There is no action required on your side. You will be notified here once IMPORTANT Please do not attempt to modify this pull request.
If you need this pull request to be removed from the queue, please contact a The following options are set: approve |
|
I have successfully merged the changeset of this pull request
Please check the status of the associated issue None. Goodbye jeanmarcmilletscality. |
4 participants
TL;DR — Feedback and Progress & loading were not grouped with the other component categories in the sidebar; their components were scattered through the loose list, below Styling. They are back in place, and "a component's guideline comes first" is now a rule the sort guarantees and a test enforces, instead of something filenames happened to produce.
📷 Screenshots
The same tree at the same viewport, with Style, Guidelines and Templates collapsed and Icon opened. On the left, the sort before this PR: Styling sits straight after Inputs, Feedback has fallen between InlineInput and AttachmentTable, Progress & loading between ButtonGroup and ConstrainedText, and Icon's attached page is called Icon and comes before Stories only because of where its file sits.
Context / Why
Two separate things were wrong with
storySort.order, and neither announced itself.The categories.
FeedbackandProgress & loadingwere written as array elements rather than as a string followed by its children:Storybook ranks a name with
order.indexOf(name), soindexOf('Feedback')was-1: the category was unranked and fell toorder.length, i.e. afterStylingandDeprecatedand tied with every component that has no category at all. The sidebar read Navigation, Data Display, Inputs, Styling, Icon, AttachmentTable, Feedback, ButtonGroup, Progress & loading, … — the two groups scattered across five positions.The per-component sub-lists.
['Checkbox', ['Guideline', '*']]and its five siblings never ran. Storybook's comparator opens withif (a.title === b.title && !options.includeNames) return 0, andincludeNameswas never set — a guideline page and its stories share a title, so the pair returned0before the list was ever consulted. Guideline-first held anyway for 16 of the 17 guidelines, because<name>.guideline.mdxsorts before<name>.stories.tsxin the same directory and the sort is stable. So the configuration carried six lines that did nothing, and the rule they were meant to express was being supplied by filenames.🧩 Approach
storySortalso accepts a comparator, which states each rule once:The comparator is a total order: two entries that differ and are both unranked fall back to their names rather than to
0. That is what makes the guideline rule hold rather than depend on where a file sits —Array.prototype.sortcannot move an entry across a run it has been told is equal, so a0there strands a page whose file is not beside its component's stories. Entries sharing a title are consequently always contiguous, and within a component the stories keep the order they were authored in.Two things had to change for the rule to be true everywhere rather than nearly:
stories/Icon.mdxfollowedCONTRIBUTING.md's recipe literally —<Meta of={IconStories} />, noname— so it was never a Guideline page at all. It now declares one, and the recipe does too. Without the name a page is named afterdocs.defaultName, takes the generated props table's place, and the component quietly loses a page instead of gaining one. The test that pins this first read the<Meta>tag as one string anchored onof=, which skipped every page writingnamefirst — and the repo writes it both ways, so 11 pages were being checked where 18 exist. It now matches the attributes independently.Components/Data Display/Charts/GlobalHealthBaris the one guideline that genuinely came last, its file being misspelledglobalheathbarrecharts.guideline.mdx. It no longer matters where the filename sorts.Visible side effect: components with no category are now alphabetical within their group — Accordion, AttachmentTable, Button, ButtonGroup, … — where they previously followed the glob's order, Accordion, Button, Icon, IconHelp, InfoMessage, InlineInput, AttachmentTable, ….
🔍 Review focus
.storybook/preview.tsx › storySort— two Storybook constraints shape how this is written, and breaking either takes the whole sidebar down rather than degrading it. The property is parsed statically, so a reference to a function defined above is rejected at index-build time with "should be defined inline"; and the extracted source is then evaluated as plain JavaScript, so TypeScript parameter annotations inside it fail withSyntaxError: Unexpected token ':'. Both surface as/index.jsonreturning 500 and an empty sidebar, never as a compile error —tsc --noEmitpasses in both broken states..storybook/preview.test.ts— the suite reads the repository rather than mocking it, in two places, and that is the point rather than an oversight.readSortSidebarpulls the arrow's source out of the file and evaluates it the way Storybook does, so the suite fails on either constraint above instead of passing against an import Storybook never performs; importing the module would also need anidentity-obj-proxythatjest.config.jsnames but the project does not install. The parameterised case walksstories/because the invariant it enforces is about the files themselves — a fixture would assert the fixture. Neither read touches a clock, the network or anything non-deterministic: both see versioned source in the working tree.🧪 How to test
npm test—.storybook/preview.test.tscovers the ordering rules, spells out all six section names and all seven category names so a misspelling cannot pass, and asserts that every MDX page attached withof={…}declaresname="Guideline". It also prints, without failing, the components that have no guideline yet.npm run storybook. Under Components, the group order is now Navigation, Data Display, Inputs, Feedback, Progress & loading, Styling, followed by the uncategorised components in alphabetical order. Deprecated ranks last but currently holds no story, so it does not appear; only the test pins its place. Before this change, Feedback and Progress & loading sat after Styling and Deprecated and were interleaved with the loose entries — compareComponents/Styling/BoxagainstComponents/Feedback/Banner, whose order is reversed by this PR.Follow-up
globalheathbarrecharts.guideline.mdxis misspelled and does not match its stories file. The sort no longer depends on it, but the filename is worth fixing on its own.docs.defaultNameinmain.ts), which reads oddly now that it sits between the guideline and the actual stories. Renaming it touches every component at once and is deliberately not in this PR.npm testnow prints the list rather than asserting on it — failing the suite would punish work nobody has been asked to do — so the backlog is visible to whoever next touches one of those files. Writing them is a separate piece of work.