chore(components): Refactor Page to be built from composed parts

[scotttjob]

Motivations

We chatted in a recent meeting about refactoring Page to be built from composed parts + using data-attributes.

Using visual regression tests as a refactor helper, I was able to refactor page to keep the exact same structure as it does today while exposing a variety of new compound pieces to build in a compositional way if required.

Each compound piece accepts data-attributes, so in the edge case where someone needs very specific data-attributes on pieces of Page, they can now accomplish that! :)

Changes

1. Page is now built from compound pieces in a composed way, and everything is exposed to the consumer for advanced applications.
2. No visual tests were modified, because they all passed! :)

Code Notes

We could prooooobably break down that ternary statement in PageTitleMeta that's deciding what to render. That still feels a bit smelly to me.

Testing

Page should work completely 100% as-is with no changes by consumers.

However, you should also now be able to build composed pages for advanced use cases as well!

Changes can be
tested via Pre-release

---

In Atlantis we use Github's built in pull request reviews.

[cloudflare-workers-and-pages]

Deploying atlantis with    Cloudflare Pages

Latest commit:	e97dd83
Status:	✅  Deploy successful!
Preview URL:	https://3739f35d.atlantis.pages.dev
Branch Preview URL:	https://cleanup-page-refactor-compou.atlantis.pages.dev

View logs

[ZakaryH]

is there a reason we'd want to stop at this level rather than continuing to a point where we have: Page.Subtitle, Page.Title, and Page.TitleMetaData?

all 3 of those feel like pieces that people would reasonably want to customize since 2/3 are already ReactNodes.

so we'd have something like

<Page.Wrapper>

</Page.Wrapper>

actually, do we even need a Page.Wrapper? or would it be possible to just have Page as the top level container?

either way, going back to what I was about to say!

<Page.Wrapper>
 <Page.Header>
  <Page.TitleBar>
   <Page.Title>
     // this is the default, you can give it different content but it'll be the predefined component
     <Page.TitleLabel><Trans>Hello friend</Trans></Page.TitleLabel>
    // this is a fully custom value that may get out of sync
    <Heading level=3><Trans>Hola Amigo</Trans></Heading>
   </Page.Title>

   <Page.TitleMetaData> 
     // doesn't seem like we have a default? so it's just a slot really
     <SomeComponent/>
   </Page.TitleMetaData>

   <Page.Subtitle> 
     // this is the default
     <Page.SubtitleLabel><Trans>This is my subtitle that gets the default appearance</Page.Subtitle>
     // this is for fully custom
     <Text><Trans>I want my subtitle to look entirely different for some reason</Trans></Text>
   </Page.Subtitle>

  </Page.TitleBar>
 // other parts
</Page.Header>
</Page.Wrapper>

I'm not 100% on the "label" naming but that's what I did in Menu so just went with the same idea.

basically we are providing the slot, that way you can customize heavily, but we also provide the default if all you want is a declarative style but are otherwise happy with what we provide

[nad182]

Implemented in e5c7132.

Page.TitleMeta has been replaced with separate Page.Title and Page.Subtitle components. Page.Wrapper is gone - Page itself is the top-level container and handles width internally.

• Page.Title accepts children (the heading text) and an optional metadata prop for badges/status labels.
• Page.Subtitle accepts either a string (gets the default Text/Emphasis/Markdown "treatment") or a ReactNode for full customization.
• I didn't go as deep as "Page.TitleLabel" / "Page.SubtitleLabel" since the string VS ReactNode pattern on children covers the same use case with less nesting.

[ZakaryH]

this getActionProps feels a bit clunky. I wonder if we couldn't find a way to abstract that or avoid people having to use this on their instances.

[nad182]

getActionProps is only used internally by the legacy renderer now.
In the composable API, Page.PrimaryAction and Page.SecondaryAction now handle their own defaults, so consumers never touch this. But if/when Button gets native ref support (as you suggested below), the need for this will likely go away.

[ZakaryH]

I feel like there's gotta be an alternate approach here.

having a visible prop with a declarative style feels redundant.

ie. if I don't want it to be visible, then wouldn't I simply not render it?

{isAdmin && <Page.PrimaryAction ...>}

[nad182]

I agree! The composable API is fully declarative now. I removed this prop.

[ZakaryH]

curious how this would work now. previously we would limit you to a primary action and a single secondary.

looking at how it works now, I assume I would be able to add as many ActionButtons as I want. maybe that's fine, but if we have opinions on how many buttons a Page should have, that would be worth documenting.

[nad182]

I replaced Page.ActionButton with moredistinct Page.PrimaryAction, Page.SecondaryAction, and Page.Menu.
Hopefully, this way we "document" our opinion on the page structure (at most these three action slots) while making each one self-documenting. I also added sensible Button defaults for both PrimaryAction and SecondaryAction, while accepting children (in case the consumers would want to provide a custom content).

[ZakaryH]

same idea for the ActionButtons, a visible prop feels odd to me. I wonder if we can avoid that and leverage the declarative style.

also kinda coming back to the topic of having N ActionButtons, it seems to me these are quite different.

one is a regular Button, and one is a Menu. while yes it will look the same at a glance, it has very different behavior.

how would someone know the default configuration? and how to build it?

I'm wondering about maybe a Page.PrimaryAction Page.SecondaryAction and Page.TertiaryAction or Page.ActionMenu

that way our opinions on how many elements should be there, in what order, and what they are is easy to understand and implement.

like with some other pieces, we'd still need to do a bit more work to create good simple defaults for people to use, and then also offer a "slot" where heavy customization can be applied - but they don't have to sort out the order/position/layout of these 3 actions.

[nad182]

Responded in the comment above.

[ZakaryH]

currently titleMetaData is optional. let's stick to that unless there's a compelling reason to make it mandatory.

https://github.com/GetJobber/atlantis/blob/master/packages/components/src/Page/Page.tsx#L37

[nad182]

I left it as optional on Page.Title.

[ZakaryH]

I'll update this comment as I progress, but immediately trying to use this compound version I hit a block with

Page

<Page>
...
</Page>

it tells me that title is a required prop, but with the sub components that's no longer the way to provide it. we'll need a way to identify the sub component invocation, and provide different props.

Subtitle
we have 2 APIs for this that I believe do the same thing

<Page.TitleMeta
 title="My Title"
 subtitle="A subtitle"
>
<Page.Subtitle>Also subtitle</Page.Subtitle>

I would be in favor of further breaking down TitleMeta so it's not 3 pieces in one, and keeping Page.Subtitle though I am realizing it's a bit hard to tell where to put it. we'll definitely need some strong docs and examples. that way, Page.Subtitle sticks around, and we get rid of the second subtitle prop.

I'm finding that the subtitle being Text > Emphasis > Markdown is pretty restrictive/opinionated. it's a sub component for sure, but I wouldn't say we have meaningfully improved composition/flexibility. it is more declarative though! assuming the Page.Subtitle invocation anyway.

Page.Wrapper

I'm not quite sure how I'm supposed to use this piece? it requires the pageStyles but I don't feel like that's something a consumer should have to worry about. it also introduces an awkward scenario due to the props required for usePage which isn't exported.

if anything I'd prefer to inverse that. where we provide it, but if you don't like it you can UNSAFE override it. actually that might be something we're missing on all the subcomponents, is the UNSAFE props.

I'd also prefer to avoid exporting styles unless we absolutely must. I find it creates tough situations for us where we can't safely change internal style details without worrying about how it will affect those using them outside.

I think it would feel much better if Page.Wrapper wasn't necessary. we just have Page that handles this. rather than having to fuss with pageStyles on Page.Wrapper you just provide Page the relevant props like width and we handle that for you.

Page.Intro

the externalIntroLinks prop is kind of awkward. I wonder if we can't make this easier to use? if I don't want a link to be external, is there a way to do that? basically I'm feeling like if I just had control over what this rendered, it would be so much more flexible. is there a reason we need to be so strict here? or could we have the default with Text + Markdown, and then have it just be a slot for when you don't want that?

Page.ActionGroup
the visible prop being required on this and the actions adds a lot of friction.

if we could just leverage the declarative nature, I think it would improve it significantly.

it's also kinda hard to know where to put this. there are so many pieces and you have to already know that the Actions are in the Header. it still renders if I put it outside Page.Header so it's up to me to know how Page is supposed to look.

I know there are so many components here that it's going to be very verbose but I wonder if we can't have it be self documenting to some degree with the names Page.Header.Actions or Page.HeaderActions.

Page.ActionButton and Page.PrimaryAction

I'm probably doing it wrong but I can't get these to sit beside each other? they keep stacking no matter what. it might be the pageStyles I'm missing? update: ah I got it, it needs to be inside the TitleBar. that's tricky and not super obvious.

the order is important here. if I don't place my primary button first, it doesn't space correctly. on one hand, I guess we can just say you should look up the order and do it that way, but on the other hand it's trivial for us to make it order independent with CSS grid. we already have access to the elements, assuming we make new unique ones for the secondary/menu actions that is.

Page.PrimaryAction accepting a Button means we don't provide a default to stay in sync with. I would prefer we did. if ever down the road we change the defaults on Page, it's simple to make sweeping changes. if each instance of Page.PrimaryAction is detached, it creates much more work. same idea applies to the other actions.

I do feel like sub components with stronger identities would be helpful. so rather than Page.ActionButton it's Page.SecondaryAction and Page.ActionMenu. I'm not sure we have a need for infinite actions? or if we even want that. I think it's perfectly valid for us to have opinions that a Page has at most 3 buttons if we have rationale to back it up. that it gets overwhelming and that's the whole point of the More Actions.

Page.ActionButton Menu

the menu behavior is difficult to implement, and you have to know to put a Menu there at all. I feel like that should be a responsibility of the (sub) component. since we don't provide the default, you have to know and repeat the exact phrasing every other "more actions" menu has on a Page.

this turns into a decent amount of markup to implement. if I want to customize it, sure that's fair. but if I just want data attributes, and the defaults I'm not sure it's fair to make people do all this. the recurring theme is for sure that we need to provide defaults IMO.

update: knowing about the TSR linking problems, I do feel it's worth spending more time to focus on the Page.Menu API, to figure out how to best allow a consumer to provide a TSR createLink'd Menu.Item that allows them to focus on what they care about, while still providing the ability for a more customized Menu if that's needed (maybe they want to wrap it in an Intercom target or PrivacyMask)

Content

largely a personal preference, I don't feel super strong about it, but I find it a bit weird to just plop my actual content inside the Page.Wrapper when everything else had a proper slot.

maybe a Page.Content or Page.Body? I think one other benefit to that outside of preference is that it allows us to target/have easier access to that wrapping element if we need to change anything, or if we eventually have other sibling sub components.

here's my final example code

const BasicTemplate: ComponentStory<typeof Page> = args => {
  const { pageStyles } = usePage({ width: "standard" });

  return (
    <Page.Wrapper pageStyles={pageStyles}>
      <Page.Header>
        <Page.TitleBar>
          <Page.TitleMeta
            title="My Title"
            subtitle="a subtitle"
            titleMetaData={
              <StatusLabel
                label={"In Progress"}
                alignment={"start"}
                status={"warning"}
              />
            }
          />
          <Page.Subtitle>check</Page.Subtitle>
          <Page.ActionGroup visible={true}>
            <Page.PrimaryAction visible={true}>
              <Button
                label="Primary Action"
                onClick={() => alert("Primary Action")}
              />
            </Page.PrimaryAction>
            <Page.ActionButton visible={true}>
              <Button
                label="Secondary Action"
                onClick={() => alert("Secondary Action")}
                type="secondary"
              />
            </Page.ActionButton>
            <Page.ActionButton visible={true}>
              <Button label="Who knows" onClick={() => alert("Who knows")} />
            </Page.ActionButton>
            <Page.ActionButton visible={true}>
              <Menu>
                <Menu.Trigger>
                  <Button label="More Actions" />
                </Menu.Trigger>
                <Menu.Content>
                  <Menu.Item
                    textValue="Action 1"
                    onClick={() => alert("Action 1")}
                  >
                    <Menu.ItemLabel>Action 1</Menu.ItemLabel>
                  </Menu.Item>
                  <Menu.Item
                    textValue="Action 2"
                    onClick={() => alert("Action 2")}
                  >
                    <Menu.ItemLabel>Action 2</Menu.ItemLabel>
                  </Menu.Item>
                  <Menu.Item
                    textValue="Action 3"
                    onClick={() => alert("Action 3")}
                  >
                    <Menu.ItemLabel>Action 3</Menu.ItemLabel>
                  </Menu.Item>
                </Menu.Content>
              </Menu>
            </Page.ActionButton>
          </Page.ActionGroup>
        </Page.TitleBar>
        <Page.Intro externalIntroLinks={true}>
          This is my intro with a link to [Jobber](https://www.jobber.com)
        </Page.Intro>
      </Page.Header>
      <Content>
        <Text>Page content here</Text>
      </Content>
    </Page.Wrapper>
  );
};

edit: thinking about it more, I wonder about the need for so many header/title sub components. would it be possible to merge or abstract one or more of these components and handle it for the consumer?

[ZakaryH]

after trying to build with these pieces, I would say there are a handful of things we should strongly consider changing and a few others that would also be worthwhile improvements. that said, this is a great start!

one final piece of FUD I have is what we are asking of consumers through all of this.

if someone has an existing Page instance, and all they want is to apply data attributes to their actions - there's no way for them to only replace that module. they are going to have to fully refactor their entire Page to this new pattern.

that's a pretty big chunk of work for a relatively small addition. something to keep in mind.

[ZakaryH]

ugh, if we get that ref support on Button done, this can go away. I was wondering why we had these extra refs that we are stripping from the props with that extra helper function.

[ZakaryH]

whoops, I forgot to try the entire point of all this 😅

I feel like giving the data attributes to Page.PrimaryAction isn't ideal. if we make the changes to Button, then data attributes can simply be given directly to that rather than this wrapping div.

a bit to sort out with the defaults cases, but for the fully custom where it's acting as a slot you'd just do <Button data-hello="world" /> and you're done! I do get that this means we might need to also do it for Menu, which is increasing the scope by a large margin, so I'm not totally against shipping without those alongside as long as we have a clear plan for how this works once those are available. I'd hate to have to immediately deprecate or otherwise change course if we get that done in the near future.

another argument for more atomic pieces for defaults that we receive the props for and can abstract with a very thin layer.

oh also Page.Header isn't applying data attributes. presumably because it's just a Content and that would require Content supporting it?

[nad182]

This follows the same overload + discriminator pattern as we have in Menu.tsx. The discriminant is title - the legacy API always requires it, the composable API uses Page.Title instead.

[nad182]

I have intentionally reverted this component to a near-exact copy of the original Page implementation (i.e. before Scott's PR/changes). The only additions I've made are ...rest / filterDataAttributes for data-attribute passthrough.

[nad182]

This would be the key piece needed for custom routing integrations (e.g. tanstack-router. We would handle the trigger button here, so consumers would only need to provide Menu.Item children.
Then in consumer apps, devs could wrap Menu.Item with router link (e.d. createLink() in case of TSR`), which would give "right-click" / "ctrl-click" support without Atlantis needing to be aware or tied to a custom routing solution.

[nad182]

Instead of relying on a 3rd party XOR, I replaced it with a native TS discriminated union using never types. This gives the same compile-time enforcement (can't pass externalIntroLinks without intro) without needing the external dependency.

[jdeichert]

I'm fine with this for now 👍

However the big caveat is that making discriminated unions by hand does not scale well IIRC. If we need to add another case, or even just another prop, it's just more work that we need to make sure we handle manually.

[nad182]

I think the complexity of adding another never or and interface would be at max equivalent to creating a new interface or type for XOR. But I'm probably not considering the scenarios that you have in mind, so I'm curious to know what type of scaling issues there are that we might face in the future.

There were discussions in the past to avoid using XOR, so that is why I wanted to substitute it with a "native" solution.

[jdeichert]

I think Button.types.ts might include some of our most complex types. But Page is less complex and I'm fine with the manual approach 👍

[jdeichert]

Not a blocker, but would love to see this file migrate to the v9 storybook!

On master, try this prompt .claude/skills/migrate-stories/SKILL.md with Cursor or whatever agent you're currently using.

[jdeichert]

Just thinking out loud, nothing to action here:

I love our visual tests, but I think we have room to improve and evolve them. Instead of one massive page which is difficult to visually diff (see video below), I think we should have a screenshot-per-test-case workflow.

There's a variety of ways we could implement that.

For example, could we rename our test file to .tsx, and write the render logic inline instead of loading pages.. just like our jest suites? (e.g. page.visual.ts -> page.visual.tsx).

Or, could we continue writing code in this file, but split it up into test cases that activate based on a url hash.. so instead of await page.goto("/visual-tests/page") we would have something like await page.goto("/visual-tests/page#composable-with-actions") and a bunch of other test cases.

---

And when I mean the screenshots are difficult to visually diff, this is what I mean.. the image is so long I can't actually tell if anything else important changed! 😅

Screen.Recording.2026-02-11.at.1.43.37.PM.mov

[nad182]

I agree! When I saw this long screenshot I was also "not impressed". So it would be great to try one of the approaches that you suggested! Just not in this PR, ideally 😅.

[jdeichert]

Is the non-composable Page truly considered legacy now?

In other cases, such as Button or Popover, we support both the old non-composable and the new composable APIs, and we don't use the "legacy" terminology.

If we're deprecating the non-composable Page, does that mean we have a bit more work to do? For example, Autocomplete v2 has a docs switcher for legacy vs supported:

Or is the legacy terminology not implying deprecation? If that's the case, I might expect the type to be called PageNonComposableProps instead of PageLegacyProps. Oh... I'm realizing we actually don't have a set pattern for this... e.g. Button has ButtonWithChildrenProps and ButtonWithoutChildrenProps and Popover has PopoverProps and PopoverProviderProps 😅. And I just noticed MenuLegacyProps which I think is where you got this strategy from!

---

Okay this is not a blocker, we can definitely ship this as is and in the future adjust the type name if we determine a better naming pattern

[ZakaryH]

so for Menu, I did call it "legacy" but that's all internal. nothing, to the best of my knowledge, references or cites "legacy" on the consumer side. I did it mostly for our sake, so it's easier to follow and make changes to the two distinct code paths. assuming I did it right, and didn't expose anything we are free to change the naming conventions internally as we please if we feel that "legacy" isn't the best way to refer to it.

I wouldn't necessarily say the old Menu stil is deprecated or "legacy" in the way that say InputTextv1 is "legacy" in the docs.

I agree, we should have a plan or conversation about how we want to position and document these component because they are starting to add up, and they don't all follow the same pattern.

[nad182]

I did follow the Menu naming convention, mainly because it was the first component that I found to reference which included both "sensible defaults" as well as a compositional pattern.
I also did spend some time going back and forth between different naming patterns, like PageBase and PageComposable, and other variations, partially because of the same concern that to some legacy might be synonymous to "deprecated". But ultimately I wanted to use a "familiar" pattern that was already present in the code base.
I'm not "married" to this specific naming, so if you guys have other stronger preference, I'm happy to change it to something more appropriate.

[jdeichert]

I know Menu does this too, but I'm unclear on what this actually provides.

Type overloads are one way to show end users different method signatures, but the other way is discriminated unions, which we already do via PageProps below: export function Page(props: PageProps). So as far as I can tell, the function overloads don't provide anything on top of that. Perhaps I'm missing some use case though 🤔

I also asked an LLM whether overloads are better for react components, and it seemed to recommend discriminated unions.. but again, perhaps its reasoning is missing context as well.

Will tag @ZakaryH since I think he'll know why Menu needed function overloads.

[ZakaryH]

the reason for the overload is to avoid this, where it is mixing items and the new invocation that has no need for that prop at all - yet the types are silent and give you no hints or indication that this is not right.

vs

I wanted to make it clear that it's a one-or-the-other. you cannot mix and match the two invocation styles.

[nad182]

⬆️ ⬆️ ⬆️ Exactly what Zak said. We get better/correct TS support to prevent us from mixing 2 different approaches to call the component that has separate/"conflicting" implementations (prop-driven VS composable).

[jdeichert]

Great, thanks for explaining!

I guess I'm confused why the discriminated union of the props (MenuLegacyProps | MenuComposableProps) is not preventing incorrect prop combinations from erroring as expected. Anyway, I'll give this a test to see it in action 👍

[ZakaryH]

it's because of children. React implicitly allows children through its default props, so that means both sets of props have children, meaning items and children can't signal that this is incorrect.

we'd have to explicitly forbid children on legacy for it to do the same thing

readonly children?: never;

trying that out, the type error is a little bit nicer. maybe that would be preferable to the overload.

that said, on Page - it does genuinely accept children as a prop so I'm not sure we'd be able to do the same thing.