Add a provider icon vocabulary for Components-owned glyphs - #313
Merged
Merged
Conversation
Components draws a glyph in sixty-six places, and six of them could be replaced: Dialog's closeIcon, TagGroup's removeIcon, Breadcrumbs' separator, and the per-instance icon on Message and on a queued toast. Everywhere else the mark is a hard-coded child. A stable part styles the element that wraps a glyph and cannot replace its children, so Display/Chip's remove sign and DataTableCore's sort arrows offer a consumer no hook at all. Adding one prop per site as each product meets each wall would mean sixty more props released one at a time, and a product with its own iconography would still pass dozens of props at dozens of call sites while missing the sites it has not met yet. The provider is where a product already says what it is: it owns locale and every Components-owned label, so it is where a product-wide icon set belongs too. CratisComponentsProvider now takes an icons map, resolved per site as per-component prop, then provider icon, then built-in glyph - the precedence messages already has for strings. A named prop addresses one call site and keeps winning; the provider speaks for the whole product. The built-in glyph stays at its call site and is deliberately absent from cratisDefaults, so a consumer that registers nothing renders exactly the markup it renders today, and a partial map leaves every name it omits alone. This registers a first cut of the vocabulary - close, remove, expand, sortAscending, sortDescending, previous, next, clear and busy - covering the concepts that repeat across components rather than naming all sixty-six sites. An icon name is public contract and harder to withdraw than to add, so the set grows as products ask for a concept by name. (#299)
Putting useCratisIcon in Source/Common/ made three per-area stylesheets swallow the whole of Common's CSS. The per-area sheets that main now publishes are derived, not written: a subpath's stylesheet is the union of the stylesheets owned by every source directory that subpath's built runtime import closure reaches, keyed on the first path segment (Source/scripts/lib/area-stylesheets.mjs). Common/ owns nine stylesheets, so an import of any module under Common/ puts all nine into the importer's sheet - whether or not the module imported has a single rule of its own. Chat, Filter and PivotViewer had reached no Common/ module before. One import of a CSS-free hook took styles.Chat.css from 5864 to 9642 gzip bytes, styles.Filter.css from 2742 to 6651, and styles.PivotViewer.css from 6840 to 10655, and the per-area budgets in verify-package-archive.mjs failed - correctly. The budget was not the problem: a hook that draws nothing must not decide what a consumer downloads, and if it may, the split that just shipped stops meaning anything the first time a shared utility lands in a component area. So the shared, CSS-free configuration seam moves to its own directory: Source/configuration/ holds CratisComponentsContext, cratisDefaults and useCratisIcon, the three modules a consuming component imports at runtime. CratisComponentsProvider stays in Common/ with the configuration types it declares, which reach configuration/ only through erased `import type` edges. The directory follows the rule types/ and renderer/ already follow - cross-cutting code that owns no CSS lives outside every area that does - and its README states it for whoever adds the next shared utility. The three sheets return to their pre-PR composition and byte size, with no budget number raised. Two sheets also got smaller than on main: styles.Dialogs.css 6582 -> 2972 and styles.Notifications.css 4988 -> 1099 gzip bytes. Dialogs and Notifications reached Common only through CratisComponentsContext, and neither renders a Common component or uses a Common-owned class, so that CSS was never theirs to ship. Their ceilings are left where they are; only the recorded measurements are updated. useCratisIcon and CratisIconResolver stay exported from the setup-only root barrel exactly as before. The incidental re-export from ./Common is dropped: the resolver is not a Common component, and the root barrel is where the PR promises it.
The root barrel is setup-only, and both the codemod and the ESLint rule keep an allowlist of the symbols a consumer may import from it; a spec asserts the barrel's exports equal that list and that the two copies agree. The icons map's type, the resolver and its hook are exported there deliberately, so they belong on the list rather than the list needing a special case.
ProgressSpinner's ring looks like an icon and is not one: it is an svg carrying the svg, track and range parts, so a registered icon replacing it would delete three parts a product may already be styling. The parts contract has to win that argument. The spinner is restored byte for byte, the loading toast keeps its own mark, and busy leaves the vocabulary — its only other site was that spinner, and a name serving one glyph in one component has not earned a place. The rule this draws is now in the documentation and in the spec that guards it: the vocabulary replaces a glyph, never an element that publishes a part. Several sites in the inventory are parted subtrees rather than glyphs, so this will come up again.
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
1 participant
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.
Summary
Components draws a glyph in 66 places; 6 of them could be replaced before this change (
Dialog.closeIcon,TagGroup.removeIcon,Breadcrumbs.separator, and the per-instanceicononMessageand on a queued toast). A stable part styles the element around a glyph and cannot replace its children, soDisplay/Chip's×andDataTableCore's▲/▼had no hook at all. Rather than add sixty more per-component props one release at a time, the provider — which already owns locale and every Components-owned label — now owns a product-wide icon vocabulary, with the per-component props kept as the single-call-site override.This is a
minor(additive provider option, unchanged defaults, unchanged props). No label applied, as requested.Added
iconsonCratisComponentsProvider, replacing Components-owned glyphs product-wide and resolved per site as per-component prop → provider icon → built-in glyph (Decide whether icons belong on the provider before every component grows its own slot #299)close,remove,expand,sortAscending,sortDescending,previous,nextandclear(Decide whether icons belong on the provider before every component grows its own slot #299)useCratisIcon()andCratisIconResolver, the resolver Components uses at every icon site, exported from the setup-only root barrel for product components that want to follow the same vocabulary (Decide whether icons belong on the provider before every component grows its own slot #299)Chip,DataTableCore's sort indicators,ComboBox,Dropdown,TablePaginator,DatePickerInput,ProgressSpinner, a toast's dismiss and loading marks,CommandForm's chips field,ChatSidebar, theChatpanel and PivotViewer's filter/detail panels now resolve their glyphs through that vocabulary instead of hard-coding them (Decide whether icons belong on the provider before every component grows its own slot #299)Documentation/Common/cratis-components-provider.mddocumentsiconsthe waymessagesis documented: the whole vocabulary with what each name draws, its default, and every component that uses it (Decide whether icons belong on the provider before every component grows its own slot #299)Review request — the vocabulary names
The mechanism is settled by the decision on #299; the names are not, and a name is harder to withdraw than to add, so this PR is for review rather than merge. Each name earns its place by being a concept more than one component draws, or one that has been asked for:
close×Dialogs/DialogImplementation.tsx:303,Notifications/Toaster.tsx:193,Chat/ChatSidebar.tsx:345,Chat/Kit/Chat.tsx:262,PivotViewer/components/DetailPanel.tsx:115— 5 sitesremove×Common/TagGroup.tsx:118,Display/Chip.tsx:51,CommandForm/fields/ChipsField.tsx:123— 3 sites.Chipis the inventory's clearest "consumer would want this and can't"expand⌄(▾inComboBox)Dropdown/DropdownImplementation.tsx:305,528,674,Common/ComboBox.tsx:395— 4 sites, 2 components, 2 different built-in glyphssortAscending▲DataTables/DataTableCore.tsx:516— 1 component, named in the decision, and previously not even styleablesortDescending▼DataTables/DataTableCore.tsx:517— sameprevious‹DataTables/TablePaginatorImplementation.tsx:76,Common/DatePickerInputImplementation.tsx:342— 2 components, same glyph, same conceptnext›DataTables/TablePaginatorImplementation.tsx:94,Common/DatePickerInputImplementation.tsx:366— sameclear×Dropdown/DropdownImplementation.tsx:326,463,549,696,Filter/FilterPanel.tsx:408— 5 sites. Deliberately not merged intoclose: dismissing a surface and emptying a value are different concepts that happen to share a glyph todaybusy◌in a toast)Display/ProgressSpinner.tsx,Notifications/Toaster.tsx:173— the decision's "busy mark"; the spinner's shape was previously unreachableOpen questions for the maintainer:
expandvschevronvsexpandCollapse.expandnames the affordance, not the shape, which is why the same name coversDropdown's⌄andComboBox's▾. If the preferred convention is to name the mark rather than the action, say so before this ships.clearvs folding it intoremove. Kept separate on purpose (see the table). Three names currently default to×; a product registering one icon for all three writes three lines.previous/nextvspreviousPage/nextPage. Kept generic so the paginator and the calendar share them. The paginator's«/»(first/last page) are not in this cut, so a product re-iconing pagination still meets two hard-coded glyphs — deliberate, since no second component draws "first"/"last".busyvsspinner/loading.busymatches the existingdata-busyattribute convention;loadingmatchesdata-loadingand the toast's ownloadingflag.messagesgroups by component (messages.dialog.close);iconsis deliberately flat, because an icon concept is shared across components while a label belongs to one. Confirm this is the intended shape before the names become contract.Verification
Source:yarn ci— 445 spec files, 1660 tests, green.node scripts/verify-api-docs.mjs— green.Storybook:yarn build && yarn verify-indexes && yarn test-storybook— 1704 story/appearance/axe cases, green. Story counts are unchanged (327 stories / 74 docs), soverify-storybook-indexes.mjsneeded no edit.Documentation/verify-markdown.sh— green.Dialog, twoTagGroups (one withremoveIcon),Chip, all threeDropdownbranches withshowClear,ComboBox,DataTableCoreunsorted/ascending/descending,TablePaginator,ProgressSpinner,DatePickerInputwith the calendar open,ChatSidebar, and a loading plus a success toast — and captureddocument.body.innerHTMLat five points. The capture was taken with this change reverted and again with it applied: both are 90,983 bytes andsha256 21d7b0cd90cb12f33c17dea0b4cb268967882e5612c0a409e907d2ec86458bc0,diffclean. The markup contains every glyph under test (×,⌄,▾,▲,▼,‹,›,◌, the spinner<svg>), so the comparison is meaningful rather than vacuously equal.Common/for_CratisComponentsProvider/when_no_icons_are_registered.tsxkeeps that property under CI.when_every_first_cut_icon_is_overridden,when_an_icon_prop_and_a_provider_icon_are_both_set("should use the provider icon for a sibling site that sets no prop") andwhen_merging_icons("should carry an element icon over by identity").when_no_icons_are_registeredpasses in both states, which is exactly its job.Not in this change
Every remaining inventory site keeps its glyph and stays unwired, because no second component draws the same concept: the paginator's
«/», the date picker's▦calendar trigger,Checkbox/CheckboxField's✓,NumberInput's−/+,PasswordField's◉/○,RatingField's★,ColumnFilterMenu's⌕,Breadcrumbs.separator,Message/Toasterseverity marks (they already take a per-instanceicon),FilterPanel's•and its CSS-border chevron (not a glyph), and the wholereact-icons-based composite set —SchemaEditor(9),Chat/Chat/Kit(9 remaining),Canvas(5),TimeMachine(4),ObjectContentEditor/ObjectNavigationalBar(4),PivotViewer's toolbar (3). Those name product actions rather than generic affordances and are a separate scoping question, as the inventory notes.Measured after review (maintainer notes)
Two review findings were folded in, and one of them turned out to be a win rather than a cost.
The configuration seam moved out of
Common. PlacinguseCratisIconinSource/Common/made Chat, Filter and PivotViewer reach theCommonarea for the first time, and because per-area stylesheets are derived from each subpath's runtime import closure (#301), their sheets swallowed the whole ofCommon's CSS — Chat 6 656 → 9 642 gzip, Filter 3 584 → 6 651, PivotViewer 7 680 → 10 655, failing 31 checks. A CSS-free hook must not carry a stylesheet, so the shared seam now lives inSource/configuration/. Every per-area budget passes without any ceiling being raised, and two areas came out smaller than onmainfor the same reason the others had grown:Dialogs6 582 → 2 972 andNotifications4 988 → 1 099, because they no longer reachCommoneither.The setup-only root allowlist.
CratisComponentsIcons,CratisIconResolveranduseCratisIconare exported from the setup-only root barrel, which both the codemod and the ESLint rule gate with an allowlist that a spec holds identical across the two copies. They are on it now.Gates after both: ESLint, Migrator and Source
yarn ci(449 files / 1 672 specs), the packed archive with all 21 per-area sheets in budget, Storybook build, index attestation and the axe matrix — all green.Still awaiting a decision on the vocabulary names before this merges.
Vocabulary reviewed: eight names, not nine
busyis out. It reads like a glyph and is not one:ProgressSpinnerdraws ansvgcarrying thesvg,trackandrangeparts, and a registered icon replacing it would silently delete three parts a product may already be styling. Where the parts contract and the icon vocabulary disagree, the parts contract wins — a consumer can restyle a parted subtree, but cannot get a deleted part back. The spinner is restored byte for byte againstmain, the loading toast keeps its own◌, and the name goes with them, since a vocabulary entry serving one glyph in one component has not earned a place.The rule that falls out is now documented and pinned by the spec that guards the vocabulary:
That matters beyond this PR: several of the inventory's 66 sites are parted subtrees rather than glyphs, and the same question will arrive with each of them.
The other eight are verified site by site to render the icon as a child of their parted element, so every part survives a registration.
sortAscendingandsortDescendingserve one component each and are kept deliberately: they are the clearest case in the inventory of a glyph a product cannot reach at all today.clearstays separate fromclosedespite sharing×today, because emptying a value and dismissing a surface are different concepts that need not share a shape forever.One inconsistency is documented rather than fixed:
expandhas two built-in glyphs,⌄on aDropdowntrigger and▾on aComboBoxtrigger. Registeringexpandmakes them consistent; registering nothing changes nothing. Unifying the defaults would move pixels for every consumer, which is a separate decision and should not ride along inside an additive change.Gates after the revision: Source
yarn ci(449 files / 1 670 specs), API-docs coverage, Migrator and ESLintci, the packed archive with the aggregate and all 21 per-area sheets in budget with no ceiling raised, Storybook build, index attestation and the axe matrix — all green.