feat(gamut): forward data-*/aria-* attributes to the right DOM nodes - #3430
Draft
dreamwasp wants to merge 11 commits into
Draft
feat(gamut): forward data-*/aria-* attributes to the right DOM nodes#3430dreamwasp wants to merge 11 commits into
dreamwasp wants to merge 11 commits into
Conversation
Pendo and other SKS tooling target elements by attribute, but nineteen components accepted data-*/aria-* at the call site and silently dropped them. TypeScript never checks hyphenated JSX attribute names, so these compiled fine and did nothing. Runtime forwarding, previously dropped entirely: Modal and Dialog (now on the role="dialog" node rather than the shroud), Overlay, Popover, Flyout, Coachmark, InfoTip, ToolTip, DataList/DataTable, Pagination, Breadcrumbs, BarChart, DatePicker, Disclosure, Video, ProgressBar, Toast, Toaster, ConnectedFormGroup and SelectDropdown. Explicit slots where the root is not the element users click, rather than re-routing existing props: buttonProps on Disclosure and InfoTip, buttonProps and dismissButtonProps on Tag, labelProps on Checkbox and Radio, inputProps on Toggle. Each is declared `never` on variants it does not apply to, so misuse is a compile error rather than a dead prop. List, DataList and DataTable place data-* on the outer wrapper, the node that already receives id, so the marked element contains the header row and not just the body. aria-label is overrideable on Pagination and Breadcrumbs, with existing defaults unchanged when nothing is passed. Modal, Dialog, Popover and Overlay deliberately keep rest spread first so role, aria-modal and data-floating still win. Adds a shared DataAttributes type for naming data-* keys in nested prop bags, and ReservedDataAttributes to stop consumers replacing data-floating, which PopoverContainer reads back via closest() for outside-click detection. In gamut-tests, setupRtl no longer counts a data-* index signature as a required prop, which would otherwise break every no-argument renderView() call on a component carrying one. Also fixes pre-existing type errors in Disclosure's test defaultProps, which used a header prop that does not exist.
|
View your CI Pipeline Execution ↗ for commit abc6c97 ☁️ Nx Cloud last updated this comment at |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3430 +/- ##
==========================================
- Coverage 91.33% 91.07% -0.26%
==========================================
Files 407 278 -129
Lines 6840 5926 -914
Branches 2227 2007 -220
==========================================
- Hits 6247 5397 -850
+ Misses 584 520 -64
Partials 9 9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
DELETE BEFORE MERGE. Isolated in its own commit so it can be dropped with a single revert. The passthrough work has no visual change anywhere, so there is nothing to eyeball in the existing stories. This renders every affected component with a probe attribute and reports, on the page, which DOM node the attribute actually landed on, so a tester does not have to work through devtools. Covers the components that previously dropped attributes, the new slot props, the List/DataTable wrapper placement including the header row, and the aria-label default-vs-override behaviour.
Broke consumer typechecking in mono (three errors in
libs/feature/admin ChargeCoupons):
Property 'component' is incompatible with index signature.
Type 'FC<ConnectedCheckboxProps>' is not assignable to type
'string | number | boolean | undefined'.
CheckboxProps is already an intersection of two unions
(CheckboxLabelUnion, CheckboxCheckedUnion). Adding a `data-*` index
signature makes Omit<CheckboxProps, ...> degrade into a signature that
constrains every property, not just data-* keys, and also produces
TS2590 "union type too complex". ConnectedFormGroup's own `field` type
does exactly that Omit, so any consumer passing `{ component, label }`
failed.
Reproduced against the published alpha in a mono checkout, confirmed
fixed by rebuilding gamut and recompiling the two files CI failed on.
Nothing is lost: `data-*` still reaches the input at runtime via the
JSX hyphenated-attribute exemption. Only labelProps needs the declared
type, because object literals don't get that exemption, and that stays.
Audited the other props types carrying DataAttributes (ListProps,
SelectDropdownCoreProps, Video) for the same Omit degradation - none
affected, since Checkbox is the only one intersecting unions.
The test that motivated the index signature now asserts ...rest reaches
the input with a declared aria-* attribute instead of data-testid.
Intersecting DataAttributes onto ComponentProps<typeof FillButton> made
AlertProps['cta'] a union too complex for downstream keyof/mapped-type
extraction (TS2590), breaking mono's ExtractableCTAProps after a
gamut-kit version bump. data-* still reaches the DOM at runtime through
the existing {...cta} spread; only the named type is gone.
Co-authored-by: Cursor <cursoragent@cursor.com>
Adds DataAttributes to DialogButtonProps (shared by both Modal and Dialog), enabling data-* attributes on primaryCta/confirmCta and the other nested CTA buttons. Safe here because DialogButtonProps is a small flat prop bag, not a type derived from a Button/Anchor union. Co-authored-by: Cursor <cursoragent@cursor.com>
…nion trap Both InfoTip.buttonProps and Disclosure.buttonProps intersected DataAttributes directly onto a union-derived ComponentProps type (the same Alert.cta trap), reproducing TS2590 for any downstream keyof/mapped-type extraction. Omitting the redundant onClick key (already overridden internally by each component after spreading the bag) before intersecting DataAttributes resolves the union without a behavior change, keeping the feature instead of dropping it. Also documents the DataAttributes union-intersection trap in utils/types.ts for future components built on Button/Anchor. Co-authored-by: Cursor <cursoragent@cursor.com>
GridFormButtons.cancel and GridForm.cancel are typed as ButtonProps, a union derived from ButtonBase (button|anchor variants), so a bare '& DataAttributes' reproduces the same TS2590 union-too-complex trap as Alert.cta - confirmed empirically, and unlike Alert/InfoTip/ Disclosure it broke GridForm.tsx itself, not just a downstream consumer. Unlike InfoTip/Disclosure, cancel's onClick isn't redundant - it's the button's real click handler, not something the component overrides internally - so it can't simply be omitted. Instead it's omitted then re-declared with ButtonBaseProps's flat, button-only onClick signature instead of ButtonProps's button|anchor union, since cancel always renders as a TextButton. This resolves the union without a behavior change, confirmed against a mono-style keyof/mapped-type extraction over cancel. GridForm.tsx now reuses GridFormButtons's exported CancelButtonProps type for its own cancel prop instead of duplicating the shape. Co-authored-by: Cursor <cursoragent@cursor.com>
…e today ConnectedFormGroupProps<T>['field'] unconditionally intersects DataAttributes onto ComponentProps<T> for any ConnectedField T - the same shape that caused the CheckboxProps mono failure. Verified empirically (a keyof/mapped-type extraction over field for each of the 6 current ConnectedField components) that none currently wrap a Button/Anchor-derived union, so this isn't broken today. Documents the risk inline so a future union-derived ConnectedField doesn't reintroduce the trap unnoticed. Co-authored-by: Cursor <cursoragent@cursor.com>
Collaborator
|
📬 Published Alpha Packages:
|
Contributor
|
🚀 Styleguide deploy preview ready! Preview URL: https://6aa2fbaed4432b8c8f4d7ff3--gamut-preview.netlify.app |
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
2 participants
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.
Overview
Pendo and other SKS tooling target elements by attribute, but nineteen Gamut components accepted
data-*/aria-*at the call site and then silently dropped them. TypeScript never checks hyphenated JSX attribute names — not justdata-, any hyphenated name — so<Modal data-marker="x" />compiled fine and did nothing. This makes the attributes actually land, on the node that matters.Runtime forwarding, previously dropped entirely. Modal and Dialog (now on the
role="dialog"node rather than the shroud, where...restwas going and being discarded twice over), Overlay, Popover, Flyout, Coachmark, InfoTip, ToolTip, DataList/DataTable, Pagination, Breadcrumbs, BarChart, DatePicker, Disclosure, Video, ProgressBar, Toast, Toaster, ConnectedFormGroup, SelectDropdown.Explicit slots where the root isn't the element users click, rather than re-routing existing props (precedent:
closeButtonPropson Modal/Dialog):buttonPropsbuttonPropsInfoTipButtonbuttonPropsTagAnchor(navigation/suggestion)dismissButtonPropsDismissButton(selection)labelProps<label>inputProps<input>, or<button>whenas="button"Each is declared
neveron variants it doesn't apply to, so misuse is a compile error rather than a dead prop.List/DataList/DataTableputdata-*on the outer wrapper, the node that already receivesid. Previously it landed on the inner<tbody>, and since<thead>is a sibling, the marker excluded the sortable column headers — the most likely thing to want to track on a table. Style props andaria-*still go to the semantic list element.aria-labelis overrideable on Pagination and Breadcrumbs (the only two that hardcoded one), with defaults unchanged when nothing is passed. Modal, Dialog, Popover and Overlay deliberately keep...restspread first sorole,aria-modalanddata-floatingstill win — every other component spreads last, per existing house style.Supporting types.
DataAttributesfor namingdata-*keys in nested prop bags (object literals don't get the JSX exemption, socta={{ 'data-marker': 'x' }}was aTS2353error).ReservedDataAttributesstops consumers replacingdata-floating, whichPopoverContainerreads back viaclosest()for outside-click detection — the attribute is also now set after{...rest}there, since ordering is the real guarantee and the type is only a guard rail.ButtonBasePropspicks up theComponentPropsWithoutRef<'button'>extension from the original spike.Also fixes pre-existing type errors in
Disclosure.test.tsx, whosedefaultPropsused aheaderprop that doesn't exist (the prop isheading) plus a non-existentwithBackground, which made everyrenderViewcall in the file fail to typecheck.Important
The second commit (
ebb7743) is a temporary test story and must be deleted before merge. It is isolated in its own commit so it can be dropped with a single revert. See the testing instructions below.PR Checklist
Testing Instructions
There is no visual change anywhere in this PR — it is attribute plumbing, so there is nothing to eyeball in the existing stories. A temporary story does the verification for you.
Go to
DELETE BEFORE MERGE / data-* passthrough (GMT-30)→Attribute Placement.It renders every affected component with a probe
data-marker, then reports on the page which DOM node each attribute actually landed on, as aPASS/FAILlist against the expected node. No devtools needed.Attribute placement: N/21 as expected.—(not rendered yet) because overlays mount on demand. Click Open Modal, then Re-check; repeat for Open Dialog and Open Flyout. All 21 rows should then readPASS.DataTablerow passes specifically — it asserts the marked node contains both<thead>and<tbody>, which is the fix for header-row targeting. It would have failed before this PR.aria-label="breadcrumbs"and the second readsaria-label="my custom trail"— default preserved, consumer value wins.Popover/PopoverContaineroutside-click still closes correctly, and that passingdata-floatingtoPopoveris a type error in your editor.Before approving: confirm commit
ebb7743(the story) has been dropped.Automated coverage: 1315 tests passing across all 9 projects,
tsconfig.lib.jsonat 0 errors,tsconfig.spec.jsonat 307 vs the 310 pre-existing baseline, andbuild-storybooksucceeds.PR Links and Envs