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:	5b18efa
Status:	✅  Deploy successful!
Preview URL:	https://fd96dd56.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?

[jdeichert]

I see PageBody wraps children in Content, but why do all of the stories/examples do that as well? Is that necessary? 🤔

<Page.Body>
  <Content>
    <Text>Page content here</Text>
  </Content>
</Page.Body>

[nad182]

You're right. That was not necessary. Not sure how I missed that, but I removed the duplication in da2a414.

[jdeichert]

Mentioned in another comment, looks like PageLegacy (and the original Page implementation) wraps their children in Content as well + consumers additionally wrap with Content too.

I guess I want to triple check here, does multiple nested Contents cause multiple levels of padding? Or are they harmless though redundant?

[jdeichert]

Off topic, not a blocker: the casting is interesting. Makes me think that filterDataAttributes and CommonAtlantisProps["dataAttributes"] should actually share the same record type.

Maybe can we log a ticket for that if it makes sense?

[nad182]

I looked into this. We have 2 approaches to handle data attributes in Atlantis:

1. explicit prop (CommonAtlantisProps): some components like Container accept dataAttributes?: { [key: data-${string}]: string } as a named prop. Values are string there because the consumer explicitly constructs the object.
2. rest spread (filterDataAttributes): some components accept ...rest and filter out data-* keys. The return type is Partial<Record<data-${string}, unknown>> because rest is can have values of any type and TS doesn't know what's in there.

Perhaps, @ZakaryH can provide more input, but I think the fix could be to either:

• change filterDataAttributes's DataAttributes type to use string instead of unknown, with a String() coercion inside the function (to satisfy the assignment), or
• change CommonAtlantisProps["dataAttributes"] to accept unknown values.

[jdeichert]

Will let Zak respond, but according to MDN, all data attribute values are strings:

Each property is a string and can be read and written
https://developer.mozilla.org/en-US/docs/Web/HTML/How_to/Use_data_attributes#javascript_access

[nad182]

That's correct. The problem is that the filterDataAttributes accepts the ...rest prop, which can have props, where values are any type. So that's why we have unknown in DataAttributes, which is used as a return type of that function:

type DataAttributes = Partial<Record<`data-${string}`, unknown>>;

function filterDataAttributes<T extends object>(props: T): DataAttributes

So const dataAttrs = filterDataAttributes(rest); returns dataAttrs of type DataAttributes, which includes unknown. But dataAttributes?: { [key: data-${string}]: string }; expects a string. That's why casting is used there.

[ZakaryH]

the CommonAtlantisProps was an early iteration; however it's not the direction we want to go anymore. we should avoid updating it if possible and instead deprecate and replace it with what we'd prefer to do.

iirc the unknown was largely to appease the types with minimal casting. I don't have any strong objections to it.

the only thing that I might want to consider at all is that it's acceptable to give non string values to data attributes in React. data-yes={true} and data-count={9} are fine. the final html will be a string, but those are not invalid as far as I'm aware.

[jdeichert]

If Heading supported data attrs, would we have included this extra div wrapper here?

This is one thing we need to keep in mind in the future: as we spread out data attrs like this, we're kind of forcing ourselves to keep it this way forever once consumers start writing code that depends on this. For example, if we happen to remove the div and move the data attrs to Header in the future, that could break consumer code where css selectors might be implicitly depending on the nested structure as it stands today. Not saying we shouldn't do this, just spreading awareness that this impacts our flexibility in the future 👍

Notably, the exact same concept and problem already exists with UNSAFE props and styles, data attrs is no different. We just need to be careful and consider future us.

Anyway, nothing to action here!

[nad182]

Yes, if Heading accepted data attributes (via ...rest + filterDataAttributes), PageTitle could pass them straight through without needing a wrapper div. I added it because I needed somewhere to put the data attributes.

I think we could improve that by "expanding" Heading props, but it's another thing that feels out of scope of Page component. Would you like me to log a ticket to allow Heading to accept data attributes?

[jdeichert]

I'm not sure if we're tracking individual components' lack of data attrs as tickets. Agreed it's out of scope and is one of those things you discover usually during implementation.

All good, nothing to action here today 👍

[ZakaryH]

this API feels a little strange

<Page.PrimaryAction label="hello">
 <Button label="goodbye" />
</Page.PrimaryAction>

"hello" does nothing but the props don't really tell me that. it's one or the other, and ideally our types/props mirror that.

also, it's a bit strange to me that we have 0 required props on our PrimaryAction. is it really ok for nothing to be provided, or do we have a minimum set of props that we ask of an instance?

[jdeichert]

Agreed.. Popover has some good examples that solve this problem. See Popover.DismissButton which is the same concept, wrapping another component by default, or allows children instead using this prop type:

export type PopoverDismissButtonProps = XOR<
  PopoverDismissButtonWithChildren,
  PopoverDismissButtonWithoutChildren
>;

[nad182]

Good catch. I adopted the similar pattern as in Popover.DismissButton - PageActionProps is now a discriminated union: you can either provide children for a custom element or label (required) for the default Button. TS will flag you if you try to do both or neither.
Done in 6e22467.

[ZakaryH]

curious about the fallback here. when would we want an empty label?

I wonder if there aren't some minimum props that we enforce via types and props. does our Page Button have different requirements than a standard Button?

[jdeichert]

Is that for label-less icon buttons perhaps? https://atlantis.getjobber.com/components/Button

Question of whether or not an icon button should be supported on Page is valid though. Even if we require label, they could provide their own children to bypass that though, which is fine from a flexibility standpoint.

[ZakaryH]

hmm I suppose that's allowed even today since our action props are referencing Button props, so it's valid to have an icon-only button.

unless we have a reason not to, we can assume that's still valid for now. oh wait, is the empty string fallback there to appease the types of Button?

if they are, and we have a good grasp of what should and should not be present on our actions, we could use the sub component Button invocation that has weaker type requirements, but would let us avoid setting fallbacks if we don't think they are realistic or useful

[jdeichert]

oh wait, is the empty string fallback there to appease the types of Button?

Ah yeah, it is required it looks like, even for icon-only buttons.

Anyway, my suggestion here would make the prop types more strict based on whether children are supplied or not, so this optional label problem would be solved by that.

[nad182]

This is no longer relevant. The label ?? "" fallback was there when label was optional. After the discriminated union change, when you're in the PageActionWithDefaults "branch", label is required, so there's no need for a fallback.
I changed how we type PageAction props in 6e22467.

[ZakaryH]

some thoughts trying to build with the Page component.

metadata

        <Page.Title metadata={<StatusLabel label="Active" status="success" />}>
          Companies
        </Page.Title>

having the meta data as a prop driven "slot" feels a bit off with the rest of the component. is there a reason we don't want a Page.TitleMetaData type component?

Page Actions

it's a little weird how we make this layout with margin on one side but I guess that's almost a benefit because it'll clearly be broken if you put them in the wrong order

PageMenu

  return (
    <div className={styles.actionButton} {...dataAttrs}>
      <Menu>
        <Menu.Trigger>
          <Button icon="more" label={triggerLabel} type="secondary" />
        </Menu.Trigger>
        <Menu.Content>{children}</Menu.Content>
      </Menu>
    </div>
  );

this is probably a fine start, but what we won't be able to do with this setup or iterate on later is if someone wants to do greater customization on the Menu. if they want access to the button, or to change things on it we'd need an entirely new component. we have no way to let <Page.Menu> offer that without breaking anything existing.

this is were I was considering breaking this up. we could have the outer div as the "slot". it handles the position, that way if you want to really customize it you can.

// Page.Tertiary
  return (
    <div className={styles.actionButton} {...dataAttrs}>
     {children}
    </div>
  );

then, we have a Page.Menu

      <Menu>
        <Menu.Trigger>
          <Button icon="more" label={triggerLabel} type="secondary" />
        </Menu.Trigger>
        <Menu.Content>{children}</Menu.Content>
      </Menu>

that way, I can do this if I want the default

<Page.Tertiary>
 <Page.Menu>
    <Menu.Item>...</Menu.Item>
  </Page.Menu>
</Page.Tertiary>

BUT if I want something customized I can do

<Page.Tertiary>
  <Menu UNSAFE_something={...} data-something="..." />
</Page.Tertiary>

but they don't have to deal with Page concerns like "where does the third interactive action go and how does it get there?"

now we don't necesarily have to optimize for this right now, but we will have a hard time making it available later AND keeping the existing Page.Menu API because they will both be trying to take that spot.

it does add another layer, and makes naming get interesting - but it splits up some of the concerns. rather than Page.Menu handling the "what" (Menu component) the "where" (rightmost spot) AND the "how" (right aligned, secondary Button Trigger'd Menu), we can make it be only the "what" and "how" which are our strong opinions on a default, and then the only other strong opinion we have if you really don't want those is there "where" via the other component.

that reminds me, I think I saw a screenshot where the alignment is off and it kinda feels bit weird to me too. we might need to update Menu to allow setting a side preference, and then apply that here.

Page Actions

same idea for these. we can keep it as Page.PrimaryAction and Page.SecondaryAction that is everything, but it will make it harder down the road if we ever want to open it up more. so it might be worth considering how strong we feel about this component, and if what we offer is the maximum amount of flexibility and customization we want to offer. if the answer is yes this is it, we will probably be fine with the components that cover everything.

but if we do that, and we change our minds later, we'll have to make new components that have overlap.

// old way
<Page.PrimaryAction>

//new way but both will exist and try to manage the position 
<Page.PrimaryActionSlot>

so it's a trade off between keeping the subcomponents to a minimum, and either taking a firm stance that we don't want to open it up - or accepting that if we do need to open it up later we are OK with a less ideal setup VS having more components, but we cover all the bases. easy to use defaults, while still keeping the door open to significant customization.

[ZakaryH]

I'm not sure about this type checking against string. what are we looking for with these?

I ask because of how translations are implemented. I might be totally fine with the default text size, styling and everything - in fact that's exactly what I want. I don't want to worry about duplicating it.

howver, I want to use a <Trans> Hola amigo</Trans> as the value.

because this isn't a string, I now have to go into Page, find what Intro would have rendered, copy that and implement it but in a way that is fragile. if Page.Intro ever updates, my copied version will NOT stay in sync

[ZakaryH]

overall, I'm not sure we should be checking for string types.

I guess I would ask what we're trying to identify with these checks?

if it's default vs custom, I would be tempted to find an alternative way that still allows passing children, and not checking the types.

[ZakaryH]

here's what might happen if I used a fragment based translation component

ignore the warnings, and imagine they are normal <Trans> components wrapping the text

I lose all the Page opinions and defaults on the text 😢 except on Title, which keeps rendering the children no matter what, and is partially what we might want to lean towards as our solution

[nad182]

I guess I would ask what we're trying to identify with these checks?

So the reason for this check is that Markdown can only accept string as its children.

Your concern about the internalization is valid, however, it would not work in case the consumers wanted to provide a markdown formatting. In legacy Page we are providing the subtitle to Markdown's content, which is always a string. So this condition mimics that. So in case of Trans, we could either let consumers provide their own styling, or do something like:

function PageSubtitle({ children, ...rest }: PageSubtitleProps) {
  const dataAttrs = filterDataAttributes(rest);

  return (
    <div className={styles.subtitle} {...dataAttrs}>
      <Text size="large" variation="subdued">
        <Emphasis variation="bold">
          {typeof children === "string" ? (
            <Markdown content={children} basicUsage={true} />
          ) : (
            children
          )}
        </Emphasis>
      </Text>
    </div>
  );
}

The typeof children === "string" condition would still be necessary, but at least the children will always be wrapped with Text and Emphasis. Would that work? (same would apply for PageIntro).

[jdeichert]

Wrapped up my review! Getting close, looks like there's a few issues to resolve with storybook and the Menu + Zak left some feedback for the API.

[jdeichert]

Looks like PageLegacy also wraps children in Content:

<Content>{children}</Content>

So is the extra Content necessary here? Do consumers normally supply Content to all Pages?

[jdeichert]

Looks like most consumers actually do supply Content themselves. And looks like even before, the old Page also did the same thing.

So I think we're good here.. just odd that double Content is required... but maybe it just works fine when nested unnecessarily?

[nad182]

I believe I have address this (or a portion of this) in a previous commit, where I removed the redundant <Content> from stories since Page.Body wraps in Content internally.

But I also asked LLM to help me understand the usage of multiple/nested Contents in Page. Basically it said that the nested Content is intentional - each level spaces different things:

• The outer Content spaces the header section from the body section
• The inner Content spaces the titlebar from the intro text
• The body Content spaces the consumer's children from each other

<div className={pageStyles}>          ← page wrapper
  <Content>                           ← outer: spaces header from body
    <Content>                         ← inner: spaces titlebar from intro
      <div className={titleBar}>...</div>
      {intro && <Text>...</Text>}
    </Content>
    <Content>{children}</Content>     ← body: spaces consumer's children
  </Content>
</div>

I looked in JFE, and the usage is really mixed... Some wrap in <Content> (Clients, Requests with custom spacing), some don't (Jobs, Timesheets).

I asked LLM if this nesting can be problematic (or redundant):

Is the double Content a problem? No. Nesting Content is by design — the margin-bottom on :not(:last-child) only affects direct children. A nested Content inside a parent Content is just one child of the parent, so the parent spaces it from siblings. The inner Content then independently spaces its own children. They don't interfere with each other.

So I guess the conclusion is that it's correct and intentional 🤷‍♂️.

[nad182]

some thoughts trying to build with the Page component.

@ZakaryH , this is a pretty thorough and large write-up and I have looked into every point you raised. I'm struggling a bit with how to respond best. We can get on a call to decide what to do with the points that you raised. But I will try my best to provide my thoughts concisely in a single comment.

---

1. metadata as a prop vs Page.TitleMetaData component

I chose the prop approach because I though that it was simpler and metadata is always a single element displayed alongside the title. Making it a sub-component would add a layer without adding flexibility - you'd still just pass a ReactNode into it. Having it as a prop is kind of like a named slot, which is a valid pattern (React Aria uses this extensively). You do have a point about consistency. So I'm happy to change it to Page.TitleMetaData if you'd prefer full consistency.

---

2. Page Actions layout / margin alignment

I used the same CSS classes, so this behaviour is inherited from whatever we have on master right now. It probably would be cleaner with gap on the action group instead of individual margins. But that's a layout change that would affect the legacy version of Page as well. If we want to do that, it would be better to do it separately (IMHO) to avoid breaking some existing usage and needing to do a full revert of all other changes that this PR has.

---

3. Page.Menu customization / slot separation

This is a big one. While I agree that from an extensibility perspective it is a vaild concern, I think we could do it later if the need arises. For the immediate need (TSR menu links), the current Page.Menu would be sufficient.
If we need to add a positional slot component later, we can restructure Page.Menu internally to use it. That way current usages would keep working and consumers who need more control would get the escape hatch.

---

4. Menu dropdown alignment

This one can be big or small depending on what we want to do. If you check Storybook, the "incorrect" alignment happens only on width: "fill" variant (on "narrow" and "standard" it matches the prop driven Page).

I was able to pinpoint it to the fact that legacy Page uses Floating UI with flip middleware (with fallbackPlacements: ["bottom-end", "top-start", "top-end"]; while the composable Page uses React Aria's AriaPopover with placement="bottom start" hard-coded (in Menu.Content) and no "flip" fallbacks configured.
We could solve it by adding placement as a prop in MenuContentComposableProps and in changing it to placement={placement ?? "bottom start"} in Menu.tsx, which would allow to configure this in Page (and any other component that uses composable Menu).
Let me know if that make sense and how you would like me to proceed.

---

5. PrimaryAction / SecondaryAction slot separation

If I understood your concern correctly, the current PageActionProps union already "solves this: when you pass children, the component just renders the "positional" wrapper div with your custom content inside. So the "slot" exists, it's just implicit rather than a separate named component. However, if you mean that you envision for the "wrapper" itself to be customizable as well (e.g. its class, element type, styles), then I guess we would need a slot + content pair. But I have a feeling that I'm just not fully or correctly understanding the issue, so I'd be happy to discuss it over a call if I'm off with interpreting it.