feat(Modal): support slot header - #1935
Merged
Merged
Conversation
Stephen Watkins (stephenjwatkins)
force-pushed
the
feat/modal-custom-header
branch
from
September 15, 2026 14:19
8266793 to
93fd93b
Compare
Stephen Watkins (stephenjwatkins)
marked this pull request as ready for review
September 15, 2026 14:53
Stephen Watkins (stephenjwatkins)
requested review from
a team
as code owners
September 15, 2026 14:53
Alex Matthews (ralexmatthews)
previously approved these changes
Sep 15, 2026
Alex Matthews (ralexmatthews)
left a comment
Contributor
There was a problem hiding this comment.
Niiiice LGTM 👌
just one non-blocking suggestion
Comment on lines
+5
to
+11
| export type ModalTitleProps = TextProps; | ||
|
|
||
| export function ModalTitle(props: ModalTitleProps) { | ||
| const modalContext = useModalContext(); | ||
| // `titleProps` spreads last so the `id` the dialog's `aria-labelledby` points | ||
| // at can't be overwritten, which would leave the modal without an accessible | ||
| // name. Everything else stays overridable. |
Contributor
There was a problem hiding this comment.
suggestion:
If we are purposely overriding the id prop, could we make the type
export type ModalTitleProps = Omit<TextProps, "id">;or something like that?
Stephen Watkins (stephenjwatkins)
force-pushed
the
feat/modal-custom-header
branch
from
September 15, 2026 16:12
93fd93b to
984ee99
Compare
Stephen Watkins (stephenjwatkins)
requested a review
from Alex Matthews (ralexmatthews)
September 15, 2026 16:12
Alex Matthews (ralexmatthews)
approved these changes
Sep 15, 2026
Alex Matthews (ralexmatthews)
left a comment
Contributor
There was a problem hiding this comment.
Siiiiick 👌
Stephen Watkins (stephenjwatkins)
deleted the
feat/modal-custom-header
branch
September 15, 2026 16:18
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.
📝 Changes
Follow-up to #1855, which added the
Modal.Footerslot and noted "Will updateModal.Headerwith similar."Modal.Headerwas fully constrained — the layout is a fixedVerticalStack→HorizontalStack, customizable only throughas/subtitle/iconAtStart/iconAtEnd. Anything else (a badge beside the title, a stepper, a two-line composite) had no escape hatch.This adds a
layoutprop.layout="custom"renderschildrenas-is, plus two subcomponents for the pieces that must stay wired to the modal:Modal.Titlecarries the modal's accessible name. It connects through context, so it works at any depth — including inside your own components.Modal.CloseButtonrenders the close icon wherever your layout needs it. No conditional required; it renders nothing when the modal isn't dismissable.The slot form is now the blessed API and the docs lead with it. The constrained props keep working unchanged, and are marked legacy in docs only — no
@deprecatedtags, no runtime deprecation warnings. At the next major,childrenalways means the slot andlayoutgoes away, convergingModal.HeaderontoDrawer.Header.Why an explicit prop rather than detecting
Modal.TitleThe footer's discriminator (
"children" in props) can't transfer: on the header,childrenis already the title. Two alternatives were considered:Modal.Titlein children. Not viable.deepFindwalks the authored JSX tree, so a consumer's own<MyHeaderRow />that rendersModal.Titleinternally is structurally invisible. The failure mode is bad — the consumer's whole tree renders inside a truncatingh2. There's a test for exactly this case.Modal.CustomHeader. No detection needed, but it permanently occupies the wrong name and costs consumers two migrations instead of one.layoutis a literal-typed discriminant, so it's a real discriminated union:<Modal.Header layout="custom" subtitle="x" />is a compile error, and the eventual major is a one-line deletion per call site.Notes for review
ModalCloseButtonis extracted and reused by the constrained branch, so there's a single implementation.HorizontalStackadds no per-child wrapper, so this is unchanged DOM. Its visuals are preserved deliberately (.closeBtn,Closeicon atsm,"Close modal"label) rather than aligned toDrawerCloseButton.Modal.TitlespreadstitlePropslast, diverging fromDrawerTitle, so a consumer passingidcan't silently unlabel the dialog. Worth back-porting toDrawerTitleseparately.console.warnwhen a custom header leaves the dialog unnamed. It checks the rendered DOM (document.getElementById(titleId)) rather than the children tree, so it has no blind spot.Drawer.Headerhas the same footgun unguarded..headerCustomis a modifier, never a replacement —.headerkeeps the padding, color, and theposition/z-indexthat keeps the scroll shadow painting above the body.ScrollShadowsWithCustomHeaderis the guard.ModalHeaderPropsnorModalFooterPropswas reachable from@easypost/easy-ui.ModalFooterPropscloses the same gap left by feat(Modal): support footer slot #1855 — flagging it so it isn't read as scope creep.documentation/specs/Modal.mdwas doubly stale — it predated feat(Modal): support footer slot #1855 and typed headerchildren/subtitleasstring. Corrected in the same pass.iconAtEndsilently suppresses the X on a dismissable modal. Left byte-identical since it's observable behavior the existing stories and tests exercise.layout="custom"is the fix — a custom header renders both an icon andModal.CloseButtonwith no conflict — so this is now a legacy-only wart with a documented migration path. Also unfixed:ModalCloseButton/DrawerCloseButtonboth omittype="button", so a header X inside a<form>submits it.Verification
Modal suite 42/42 (9 new tests); full suite 598 passed / 2 skipped.
Modal.nesting.test.tsxneeded no changes — every usage lands in the constrained branch, which is the cheapest proof of backwards compatibility.maincurrently can't run ESLint,tsc --build, or Storybook — the Storybook 10 upgrade (on its own branch) is what fixes all three. Typecheck error count is identical before and after this change (131, all pre-existing), andbuild:types, fulllint, and Storybook were verified green on a throwaway branch with those fixes merged in.Still needs a human eye, hence the unchecked boxes below: pixel comparison of
Completeagainstmain— specificallyiconAtEndat2xl, which relies on a negative-margin rule — plus the two scroll-shadow stories, the axe pass, and cross-browser. Chromatic covers the visual diff once the Storybook fixes land.✅ Checklist