-
Notifications
You must be signed in to change notification settings - Fork 662
🚧 [WIP] Dialog 4 layer specs #7778
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 16 commits
bd94618
e3e8fdf
696c70c
b658628
741f7f0
8009b18
4856197
1dcab8f
87742f1
442b695
2403fe2
220512d
8cf7677
3aaee1f
cb24f6c
6c8e98e
aecd3f2
413409f
4be8dd7
8b1a340
26e532f
a5e5b64
8964d48
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,277 @@ | ||
| # Modular Component Architecture | ||
|
|
||
| 📆 Date: 2026-04-27 | ||
|
|
||
| ## Status | ||
|
|
||
| | Stage | State | | ||
| | -------------- | -------------- | | ||
| | Status | Proposed 🟡 | | ||
| | Implementation | In progress ⚠️ | | ||
|
|
||
| ## Context | ||
|
|
||
| Primer React components are monolithic — each ships as a single unit mixing behavior, accessibility, styling, and composition into one API surface. This makes it difficult for consumers to: | ||
|
|
||
| - Use Primer accessibility and behavior logic without Primer's visual opinions | ||
| - Compose components from smaller, reusable parts | ||
| - Incrementally adopt Primer in codebases with existing design systems | ||
| - Override specific layers (e.g., styling) without forking the entire component | ||
|
|
||
| We need a layered architecture where each layer has a clear responsibility, a stable API contract, and can be used independently. | ||
|
|
||
| **Source issues:** | ||
|
|
||
| - [primer#6546](https://github.com/github/primer/issues/6546) — Layer definitions | ||
| - [core-ux#2270](https://github.com/github/core-ux/issues/2270) — Layer 2 composition pattern | ||
| - [core-ux#2272](https://github.com/github/core-ux/issues/2272) — Layer 3/4 prop-getters vs context | ||
| - [core-ux#2269](https://github.com/github/core-ux/issues/2269) — Export & package structure | ||
| - [core-ux#2271](https://github.com/github/core-ux/issues/2271) — Contracts between layers | ||
|
|
||
| ## Decision | ||
|
|
||
| ### Four layers | ||
|
|
||
| Every modular component is decomposed into four layers. Each layer builds on the one below. | ||
|
|
||
| | Layer | Name | Responsibility | Styled? | | ||
| | ----- | ----------- | ----------------------------------------------------- | ---------------------------- | | ||
| | 4 | Hooks | Individual, single-purpose behavior | ❌ No markup or styles | | ||
| | 3 | Foundations | Component-specific ARIA wiring + behavior composition | ❌ Unstyled (CSS reset only) | | ||
| | 2 | Parts | Primer-styled JSX composition | ✅ Full Primer styles | | ||
| | 1 | Ready-made | Props-based convenience wrapper | ✅ Full Primer styles | | ||
|
|
||
| Ready-made (L1) uses Parts (L2), Parts use Foundations (L3), Foundations use Hooks (L4). | ||
|
|
||
| ### Layer 4 — Hooks | ||
|
|
||
| **Individual, single-purpose behavior hooks.** Not component-specific. Reusable across any component that needs the behavior. | ||
|
|
||
| Examples: `useFocusTrap`, `useFocusZone`, `useOnEscapePress`, `useScrollLock` | ||
|
|
||
| **API pattern:** Each hook takes options and returns refs, callbacks, or prop objects. | ||
|
|
||
| ```tsx | ||
| const {containerRef} = useFocusZone({bindKeys: FocusKeys.ArrowVertical}) | ||
| ``` | ||
|
|
||
| **Rules:** | ||
|
|
||
| - One behavior per hook — no compound hooks at this layer | ||
| - No knowledge of which component is consuming them | ||
| - No styling or markup opinions | ||
|
|
||
| ### Layer 3 — Foundations | ||
|
|
||
| **Compound hooks returning prop-getters.** Component-specific. Wires up ARIA relationships, composes Layer 4 hooks, manages component lifecycle. | ||
|
|
||
| **Key rule:** Prop-getters are the public API. Context is an implementation detail only — consumers never call `useContext()` directly. | ||
|
|
||
| ```tsx | ||
| // Foundation consumer — owns all markup | ||
| const dialog = useDialogFoundation({open, onClose}) | ||
|
|
||
| <dialog {...dialog.getDialogProps()}> | ||
| <h2 {...dialog.getTitleProps()}>Title</h2> | ||
| <p {...dialog.getDescriptionProps()}>Subtitle</p> | ||
| <div {...dialog.getBodyProps()}>Content</div> | ||
| <button {...dialog.getCloseProps()}>✕</button> | ||
| </dialog> | ||
| ``` | ||
|
|
||
| **Why prop-getters over components:** | ||
|
|
||
| - Full markup ownership — consumer chooses every element | ||
| - Composable with any component system (Radix, custom, etc.) | ||
| - Multi-element wiring is natural (`getTitleProps()`, `getBodyProps()`) | ||
| - TypeScript return types are explicit and statically known | ||
| - No imposed component tree | ||
|
|
||
| **Foundation CSS:** Each foundation may ship a minimal CSS reset that removes browser defaults without adding visual opinion. Use `:where()` for zero specificity so consumer styles always win. | ||
|
|
||
| **Context** is allowed internally for ARIA cross-wiring (e.g., `aria-labelledby` pointing title ID to dialog) but is never exposed to consumers. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a convention for providing the context root? Would this be a component? (e.g. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea I think |
||
|
|
||
| ### Layer 2 — Parts (Composition) | ||
|
|
||
| **Styled JSX components for Primer-opinionated composition.** | ||
|
|
||
| **Composition via slots (`useSlots`):** | ||
|
|
||
| - Use slots (children-based) for all composition | ||
| - Render props exist only in legacy code — do not add new ones | ||
| - Context (e.g., `useDialogContext()`) replaces render-prop-injected IDs for ARIA wiring | ||
| - Never use `React.Children` + `React.cloneElement` | ||
|
|
||
| ```tsx | ||
| // Parts consumer — Primer-styled, compositional | ||
| <DialogParts.Root open={open} onClose={onClose}> | ||
| <DialogParts.Content width="large"> | ||
| <DialogParts.Header> | ||
| <DialogParts.Title>Title</DialogParts.Title> | ||
| <DialogParts.CloseButton /> | ||
| </DialogParts.Header> | ||
| <DialogParts.Body>Content</DialogParts.Body> | ||
| <DialogParts.Footer> | ||
| <Button>Cancel</Button> | ||
| </DialogParts.Footer> | ||
| </DialogParts.Content> | ||
| </DialogParts.Root> | ||
| ``` | ||
|
|
||
| **Rules:** | ||
|
|
||
| - Parts use Foundations internally — they call `useComponentFoundation()` and spread prop-getters | ||
| - Parts add Primer design tokens, CSS modules, and layout opinions | ||
| - Parts are the building blocks for Ready-made (Layer 1) | ||
| - All Parts must include `data-component` attributes per [ADR-023](./adr-023-stable-selectors-api.md) | ||
|
|
||
| ### Stable selectors (ADR-023) | ||
|
|
||
| All Layer 2 Parts and Layer 1 Ready-made components must include `data-component` attributes as defined in [ADR-023](./adr-023-stable-selectors-api.md). | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let me know if this is covered in the ADR, but if we're providing parts/slots why would we need
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @joshblack I added this because we have it now. If we agree on this we can think about if this is made completely redundant, or not. |
||
|
|
||
| **Rules:** | ||
|
|
||
| - Root component: `data-component="ComponentName"` (e.g., `data-component="Dialog"`) | ||
| - Sub-components match the React API: `data-component="ComponentName.PartName"` (e.g., `data-component="Dialog.Header"`) | ||
| - State and modifier attributes (`data-width`, `data-size`, `data-variant`) remain separate — they describe state, not identity | ||
| - Layer 3 (Foundations) does NOT add `data-component` — the consumer owns all markup | ||
| - Internal CSS may target `data-component` selectors using `:where()` for zero specificity | ||
|
|
||
| ```html | ||
| <!-- Layer 2 example: all parts have stable identifiers --> | ||
| <dialog data-component="Dialog"> | ||
| <div data-component="Dialog.Content" data-width="large" data-position-regular="center"> | ||
| <header data-component="Dialog.Header"> | ||
| <h2 data-component="Dialog.Title">Title</h2> | ||
| <button data-component="Dialog.CloseButton">✕</button> | ||
| </header> | ||
| <div data-component="Dialog.Body">Content</div> | ||
| <footer data-component="Dialog.Footer">...</footer> | ||
| </div> | ||
| </dialog> | ||
| ``` | ||
|
|
||
| ### Layer 1 — Ready-made | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want this for all components? This is an open question I have if this is simpler than the alternatives (I just don't know). Sometimes the config approaches can lead to massive types and assume it's what makes some components more unwieldy (like select panel) Maybe asked another way, are there limitations on the scope of this API that we would like to put on a component? Should everything be available through this or is this capturing the 80% use-case? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it should be mandatory for every component. To be honest I could see a world where L1 doesn't exist at all and essentially we just point people to a cookbook/recipes for common use cases which compose L2 parts into various standalone components. For the same reasons you described I'm not a fan of config/prop-driven components either. Sometimes a huge number of props, often taking objects as props which then causes a cascade of unnecessary renders if people forget to memoize their objects. I don't feel we'd actually lose anything by ditching L1 entirely. There might be cases where it really makes sense, but I'd always prefer slightly more verbose+composable than stuffing everything into a single component's props. When I first suggested Layer 1 it was more as a way to make the proposal a bit more approachable than jumping straight to composable parts as the default. If eng is open to it though we can explore what it might look like to not have L1 by default and then see if there are any scenarios in which we feel it adds genuine value. |
||
|
|
||
| **Props-based convenience API.** The simplest way to use a component — pass data, get a fully composed component. | ||
|
|
||
| ```tsx | ||
| // Ready-made consumer — just props | ||
| <Dialog | ||
| open={open} | ||
| onClose={onClose} | ||
| title="Confirm" | ||
| footerButtons={[ | ||
| {buttonType: 'default', content: 'Cancel', onClick: onClose}, | ||
| {buttonType: 'primary', content: 'Save', onClick: onSave}, | ||
| ]} | ||
| > | ||
| Are you sure you want to save? | ||
| </Dialog> | ||
| ``` | ||
|
|
||
| **Rules:** | ||
|
|
||
| - Ready-made is a thin wrapper over Parts — it composes `<Component.Root>`, `<Component.Header>`, etc. | ||
| - Props map directly to Parts children — no new behavior at this layer | ||
| - This is the default recommendation for most consumers | ||
|
|
||
| ## Export & package structure | ||
|
|
||
| ### Entry points | ||
|
|
||
| | Layer | Stable import | Experimental import | | ||
| | --------------- | --------------------------- | ---------------------------------------- | | ||
| | 1 — Ready-made | `@primer/react` | `@primer/react/experimental` | | ||
| | 2 — Parts | `@primer/react` | `@primer/react/experimental` | | ||
| | 3 — Foundations | `@primer/react/foundations` | `@primer/react/foundations/experimental` | | ||
| | 4 — Hooks | `@primer/react/hooks` | `@primer/react/hooks/experimental` | | ||
|
Comment on lines
+245
to
+246
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. More of a meta/organization comment, happy to drop more entry points and just use an
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fine with this. I think this is something where we are very happy to just follow primer eng's lead. |
||
|
|
||
| ### Naming conventions | ||
|
|
||
| | Layer | Convention | Example | | ||
| | ----- | -------------------------- | ---------------------------------------- | | ||
| | 4 | `use<Behavior>` | `useScrollLock`, `useFocusTrap` | | ||
| | 3 | `use<Component>Foundation` | `useDialogFoundation` | | ||
|
joshblack marked this conversation as resolved.
Outdated
|
||
| | 2 | `<Component>Parts.<Part>` | `DialogParts.Root`, `DialogParts.Header` | | ||
| | 1 | `<Component>` | `Dialog` | | ||
|
|
||
| ### Rules | ||
|
|
||
| - `@primer/react` does NOT re-export Foundations or Hooks — each layer is opt-in via its own entry point | ||
| - All layers ship in one package version | ||
| - Stability is per-component — `useDialogFoundation` can graduate while others remain experimental | ||
| - Graduation = one-time import path change (`/experimental` → stable) | ||
|
|
||
| ### Source folder structure | ||
|
|
||
| ``` | ||
| packages/react/src/ | ||
| ├── hooks/ # Layer 4 (existing + new) | ||
| │ ├── useFocusTrap.ts | ||
| │ ├── useOnEscapePress.ts | ||
| │ └── useScrollLock.ts | ||
| ├── foundations/ # Layer 3 | ||
| │ └── experimental/ | ||
| │ └── <Component>/ | ||
| │ ├── use<Component>Foundation.ts | ||
| │ └── index.ts | ||
| ├── experimental/ # Layer 2 + Layer 1 (while experimental) | ||
| │ └── <Component>/ | ||
| │ ├── <Component>.tsx # Parts (Layer 2) | ||
| │ ├── <ReadyMade>.tsx # Ready-made (Layer 1) | ||
| │ ├── <Component>.module.css | ||
| │ ├── <Component>.spec.md # Component specification | ||
| │ └── index.ts | ||
| └── <Component>/ # Layer 1 + 2 (after graduation) | ||
| └── <Component>.tsx | ||
| ``` | ||
|
|
||
| ### package.json exports (additions for new entry points) | ||
|
|
||
| ```json | ||
| { | ||
| "./foundations/experimental": { | ||
| "types": "./dist/foundations/experimental/index.d.ts", | ||
| "default": "./dist/foundations/experimental/index.js" | ||
| }, | ||
| "./hooks/experimental": { | ||
| "types": "./dist/hooks/experimental/index.d.ts", | ||
| "default": "./dist/hooks/experimental/index.js" | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| ## Alternatives considered | ||
|
|
||
| ### Components instead of prop-getters for Layer 3 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would love the option to ship both, potentially. One thing I would be concerned about is that there is a claim that if you use these foundations then what you build is accessible. However, that may only be true if specific criteria are met (one could be that headings must be a descendant of the dialog or aria-modal node). This could be more easily enforced through components than the hooks API (or at least that's my assumption). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Totally agree, I think this is an oversight in the current ADR rather than a deliberate choice. I'll update the ADR to reflect this. Regarding accessibility, I had the same thought and I think it needs to be very explicitly communicated what is handled by each layer and what the responsibilities of the implementer are at each layer. I'm envisioning an accessibility checklist for each layer of particular things that we know an implementer must still do if they choose to use a component at a particular layer. |
||
|
|
||
| We considered shipping unstyled React components (like Radix primitives) at Layer 3. This was rejected because: | ||
|
|
||
| - It imposes a component tree — consumers must nest `<DialogTitle>` inside `<DialogRoot>` | ||
| - Harder to compose with non-React systems or existing component libraries | ||
| - Prop-getters give the consumer full control over every rendered element | ||
|
|
||
| ### Context as public API | ||
|
|
||
| We considered exposing React Context for ARIA wiring (e.g., `useDialogContext()` to get title IDs). This was rejected because: | ||
|
|
||
| - It leaks implementation details and couples consumers to our component tree | ||
| - Prop-getters achieve the same wiring without requiring a specific provider hierarchy | ||
| - Context is still used internally — just not exposed to consumers | ||
|
|
||
| ### Render props for Layer 2 composition | ||
|
|
||
| Render props were considered for Layer 2 but rejected: | ||
|
|
||
| - They already exist in legacy code — we don't want to add more | ||
| - Slots via `useSlots` are more declarative and composable | ||
| - `React.Children` + `React.cloneElement` are fragile and discouraged by React team | ||
|
|
||
| ## Consequences | ||
|
|
||
| - Every new component should be built using this 4-layer decomposition | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a scary sounding line but I think I can get behind it if the idea is:
So if I'm making a Tabs component I would:
One thing that I'm not sure about is if we would necessarily want config for every component. I'm not sure if, in all cases, that is easier to use than the slot style. Curious what other people thing 👀 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 to your tabs breakdown, that's exactly the mental model I have. I touched on the config point in another comment too. TL;DR there is we might not need it |
||
| - Existing components can be incrementally migrated by extracting hooks and foundations | ||
| - Consumers get predictable, documented layers to adopt at their comfort level | ||
| - Breaking changes can be scoped to individual layers rather than entire components | ||
| - The first component through this architecture is Dialog — it serves as the reference implementation | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One idea for this instead of
:where()is that these live in an earlier layer in the css cascade layer list (or just lives in a css cascade layer)