feat: improve slot system consistency#7869
Open
adierkens wants to merge 1 commit into
Open
Conversation
- Remove orphan __SLOT__ markers from root components with no consumers:
ActionMenu (Menu), UnderlinePanels, PageLayout, SegmentedControl,
RadioGroup, CheckboxGroup, and Dialog.
- Standardize Symbol() descriptions to the Parent.Slot convention:
CheckboxOrRadioGroup.{Label,Caption,Validation}, Tooltip (was
DEPRECATED_Tooltip), and DataTable.Table (was Table).
- Migrate PageHeader, NavList.Item, and the internal CheckboxOrRadioGroup
to useSlots instead of hand-rolled Children.toArray + isSlot loops.
The CheckboxOrRadioGroup migration also drops duplicated work where
useSlots was already called but slots were re-extracted by hand.
- Export useSlots, isSlot, asSlot, WithSlotMarker, and FCWithSlotMarker
publicly from @primer/react (also useSlots from @primer/react/hooks).
- Add asSlot(component, slotSource) helper that copies a __SLOT__ marker
from a source slot component onto a wrapper, replacing the cast-heavy
manual marker-copy pattern. Dev-mode warns when the source has no marker.
- Add a dev-mode displayName-mismatch warning in useSlots that catches
wrappers around slot components that forgot to copy the marker.
- Add .github/skills/slots/SKILL.md documenting when to use slots, naming
conventions, public APIs, and common pitfalls. Reference it from
copilot-instructions.md.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🦋 Changeset detectedLatest commit: 54ebf65 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
Bot
added
the
integration-tests: recommended
Contributor
|
adierkens
added
the
Canary Release
13 tasks
adierkens
added
Canary Release
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves consistency and ergonomics of Primer React’s slot system by standardizing __SLOT__ markers, migrating a few internal consumers to useSlots, and exposing slot primitives as public API (plus a new asSlot helper). It also adds contributor documentation for slots and updates the public exports snapshot accordingly.
Changes:
- Add/export slot utilities (
useSlots,isSlot,asSlot) and slot marker types (WithSlotMarker,FCWithSlotMarker) from@primer/react(+useSlotsfrom@primer/react/hooks), with tests and export snapshot updates. - Standardize slot marker symbol descriptions (e.g.
Parent.Slotconvention) and remove orphan root__SLOT__markers from components that aren’t scanned as children. - Improve slot usage consistency by migrating selected components to
useSlotsand adding a new dev-only warning path inuseSlots.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/utils/as-slot.ts | Adds asSlot helper to copy __SLOT__ markers onto wrapper components. |
| packages/react/src/utils/tests/as-slot.test.tsx | Adds unit tests for asSlot marker copying + dev warning. |
| packages/react/src/hooks/useSlots.ts | Adds a dev-only warning for displayName matches that are missing a __SLOT__ marker. |
| packages/react/src/hooks/index.ts | Re-exports useSlots from the hooks entrypoint. |
| packages/react/src/index.ts | Publicly exports slot primitives/types (useSlots, isSlot, asSlot, etc.). |
| packages/react/src/tests/snapshots/exports.test.ts.snap | Updates exports snapshot for newly public exports. |
| packages/react/src/PageHeader/PageHeader.tsx | Migrates slot detection from manual Children traversal to useSlots for dev-only validation logic. |
| packages/react/src/NavList/NavList.tsx | Migrates SubNav/TrailingAction extraction to useSlots. |
| packages/react/src/internal/components/CheckboxOrRadioGroup/CheckboxOrRadioGroup.tsx | Removes redundant manual slot extraction in favor of the existing useSlots result. |
| packages/react/src/internal/components/CheckboxOrRadioGroup/CheckboxOrRadioGroupLabel.tsx | Standardizes slot symbol description to CheckboxOrRadioGroup.Label. |
| packages/react/src/internal/components/CheckboxOrRadioGroup/CheckboxOrRadioGroupCaption.tsx | Standardizes slot symbol description to CheckboxOrRadioGroup.Caption. |
| packages/react/src/internal/components/CheckboxOrRadioGroup/CheckboxOrRadioGroupValidation.tsx | Standardizes slot symbol description to CheckboxOrRadioGroup.Validation. |
| packages/react/src/Tooltip/Tooltip.tsx | Renames Tooltip slot symbol description to Tooltip. |
| packages/react/src/DataTable/index.ts | Renames Table slot symbol description to DataTable.Table. |
| packages/react/src/ActionMenu/ActionMenu.tsx | Removes orphan root __SLOT__ marker from ActionMenu root. |
| packages/react/src/Dialog/Dialog.tsx | Removes orphan root __SLOT__ marker from Dialog root. |
| packages/react/src/PageLayout/PageLayout.tsx | Removes orphan root __SLOT__ marker from PageLayout root. |
| packages/react/src/SegmentedControl/SegmentedControl.tsx | Removes orphan root __SLOT__ marker from SegmentedControl root. |
| packages/react/src/RadioGroup/RadioGroup.tsx | Removes orphan root __SLOT__ marker from RadioGroup root. |
| packages/react/src/CheckboxGroup/CheckboxGroup.tsx | Removes orphan root __SLOT__ marker from CheckboxGroup root. |
| packages/react/src/experimental/UnderlinePanels/UnderlinePanels.tsx | Removes orphan root __SLOT__ marker from UnderlinePanels root (retains sub-slots). |
| .github/skills/slots/SKILL.md | Adds contributor documentation (“skill”) for slot conventions and public APIs. |
| .github/copilot-instructions.md | References the new slots skill doc from Copilot instructions. |
| .changeset/slot-system-consistency.md | Adds a minor changeset describing slot system API + consistency changes. |
Copilot's findings
- Files reviewed: 24/24 changed files
- Comments generated: 3
Comment on lines
+22
to
+28
| if (__DEV__) { | ||
| warning( | ||
| !slotSource.__SLOT__, | ||
| 'asSlot: the provided slotSource does not have a `__SLOT__` marker. The wrapper will not be recognised by the parent slot scanner.', | ||
| ) | ||
| } | ||
| ;(component as unknown as SlotMarker).__SLOT__ = slotSource.__SLOT__ |
Comment on lines
+16
to
+20
| beforeEach(() => { | ||
| warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) | ||
| return () => { | ||
| warnSpy.mockRestore() | ||
| } |
| }) | ||
| ``` | ||
|
|
||
| `isSlot` always checks both the direct type and the slot marker so wrappers built with `asSlot` are recognised. |
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.
Closes #
Improves consistency across the slot system (
__SLOT__markers +useSlots/isSlot). Three buckets of work:Symbol(...)descriptions, migrate hand-rolledChildren.toArray + isSlotconsumers touseSlotswhere the pattern fits.asSlothelper that replaces the cast-heavy marker-copy pattern..github/skills/slots/SKILL.mdthat describes when and how to use slots, with naming conventions and pitfalls.Changelog
New
@primer/react:useSlots,isSlot,asSlot,WithSlotMarker,FCWithSlotMarker(alongside the existingSlotMarkertype).useSlotsis also exported from@primer/react/hooks.asSlot(component, slotSource)helper that copies a__SLOT__marker from a source slot component onto a wrapper. Typed; dev-warns when the source has no marker. Replaces the cast-heavy(Wrapper as typeof Wrapper & {__SLOT__?: symbol}).__SLOT__ = Source.__SLOT__pattern.useSlotswhen a child'sdisplayNamematches a slot component'sdisplayNamebut the child is missing the__SLOT__marker (the most common wrapping footgun)..github/skills/slots/SKILL.mdcontributor skill documenting when to use slots, naming conventions, public APIs, limitations, and common pitfalls. Referenced from.github/copilot-instructions.md.Changed
Symbol(...)descriptions used as slot markers to theParent.Slotconvention:CheckboxOrRadioGroup.Label,CheckboxOrRadioGroup.Caption,CheckboxOrRadioGroup.Validation,Tooltip(wasDEPRECATED_Tooltip),DataTable.Table(wasTable).PageHeader,NavList.Item, and the internalCheckboxOrRadioGroupto useuseSlotsinstead of hand-rolledReact.Childrentraversals. TheCheckboxOrRadioGroupmigration also removes duplicated work whereuseSlotswas already being called but slots were re-extracted by hand immediately after.Removed
__SLOT__markers from 7 root components with no internal consumers:ActionMenu(rootMenu),UnderlinePanels,PageLayout,SegmentedControl,RadioGroup,CheckboxGroup, andDialog. Sub-component markers are intentionally retained so consumers can keep wrapping them.What was evaluated but intentionally not migrated
Four hand-rolled consumers use patterns that don't fit
useSlots's single-match-per-key model:CheckboxGroupandUnderlinePanels— multi-match (find all children of a given type). Would migrate cleanly ifuseSlotsgrew a multi-match option; design TBD in a follow-up.SegmentedControl— per-child classification with index tracking (selection state). Not a slot extraction pattern.ActionMenu—Children.mapwith side effects, including grandchild introspection for Tooltip-wrapped Anchors. Not a slot extraction pattern.TreeView'suseSubTreeis also a clean single-match consumer but lives inside auseMemo; migrating trippedreact-hooks/rules-of-hooks(the lint rule enforces theuse*naming convention even thoughuseSlotsis a plain function under the hood). Left as-is. The slots skill calls this pitfall out explicitly.Rollout strategy
New public exports + new helper, no breaking changes. The removed root
__SLOT__markers had no consumers anywhere in the codebase (verified by grep) so removing them is non-breaking; if a downstream consumer was secretly wrapping against e.g.Dialog.__SLOT__, they would now lose the slot match — but Dialog is a root and nothing scanned for it, so there was nothing to be matched into.Testing & Reviewing
prettier --checkall clean on changed files.exports.test.tssnapshot was updated to include the new public exports.Areas worth a close look during review:
packages/react/src/hooks/useSlots.ts— the newwarnIfDisplayNameMatchesWithoutMarkerhelper is dev-only but runs once per non-matching child; happy to gate it behind a feature flag if perf is a concern.packages/react/src/PageHeader/PageHeader.tsx—useSlotsis now called at the top level even though the only consumer is a dev-only effect. Couldn't keep it inside the effect because ofreact-hooks/rules-of-hooks. PageHeader isn't render-hot so this should be fine, but flagging..github/skills/slots/SKILL.md— convention statements; let me know if anything contradicts your intent.Merge checklist