chore(components): Refactor Page to be built from composed parts#2867
chore(components): Refactor Page to be built from composed parts#2867
Conversation
Deploying atlantis with
|
| Latest commit: |
7ebc98a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://605af9f8.atlantis.pages.dev |
| Branch Preview URL: | https://cleanup-page-refactor-compou.atlantis.pages.dev |
| <Page.TitleMeta | ||
| title={title} | ||
| titleMetaData={titleMetaData} | ||
| subtitle={subtitle} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.Titleaccepts children (the heading text) and an optionalmetadataprop for badges/status labels.Page.Subtitleaccepts either astring(gets the default Text/Emphasis/Markdown "treatment") or aReactNodefor 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.
| ref={primaryAction?.ref} | ||
| visible={!!primaryAction} | ||
| > | ||
| <Button {...getActionProps(primaryAction)} fullWidth /> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| <Page.ActionGroup visible={!!showActionGroup}> | ||
| <Page.PrimaryAction | ||
| ref={primaryAction?.ref} | ||
| visible={!!primaryAction} |
There was a problem hiding this comment.
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 ...>}
There was a problem hiding this comment.
I agree! The composable API is fully declarative now. I removed this prop.
| > | ||
| <Button {...getActionProps(primaryAction)} fullWidth /> | ||
| </Page.PrimaryAction> | ||
| <Page.ActionButton |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
| type="secondary" | ||
| /> | ||
| </Page.ActionButton> | ||
| <Page.ActionButton visible={!!showMenu}> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Responded in the comment above.
| ...rest | ||
| }: { | ||
| readonly title: ReactNode; | ||
| readonly titleMetaData: ReactNode; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I left it as optional on Page.Title.
|
I'll update this comment as I progress, but immediately trying to use this compound version I hit a block with Page it tells me that Subtitle 
I would be in favor of further breaking down I'm finding that the Page.Wrapper I'm not quite sure how I'm supposed to use this piece? it requires the if anything I'd prefer to inverse that. where we provide it, but if you don't like it you can 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.Intro the Page.ActionGroup 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 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.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 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.
I do feel like sub components with stronger identities would be helpful. so rather than 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 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 maybe a here's my final example codeedit: 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? |
|
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 that's a pretty big chunk of work for a relatively small addition. something to keep in mind. |
| import { type SectionProps } from "../Menu"; | ||
|
|
||
| export type ButtonActionProps = ButtonProps & { | ||
| ref?: React.RefObject<HTMLDivElement | null>; |
There was a problem hiding this comment.
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.
|
whoops, I forgot to try the entire point of all this 😅 I feel like giving the data attributes to 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 another argument for more atomic pieces for defaults that we receive the props for and can abstract with a very thin layer. oh also |
- Uses the same TS overload pattern as Menu - Replaces ts-xor XOR with native discriminated union - Props-based usage is fully preserved; composable mode activates when title is not passed as a prop - Page.Menu uses the composable Menu internally, enabling consumers to provide Menu.Item children compatible with client-side routers - PrimaryAction/SecondaryAction provide sensible Button defaults but accept children for custom elements - All compound components support data-attributes via filterDataAttributes
- Creates PageNotes.mdx covering all sub-components, the slot pattern for custom actions, and an explanation of TSR integration via createLink(Menu.Item).
@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.
I chose the prop approach because I though that it was simpler and
I used the same CSS classes, so this behaviour is inherited from whatever we have on
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
This one can be big or small depending on what we want to do. If you check Storybook, the "incorrect" alignment happens only on I was able to pinpoint it to the fact that legacy Page uses Floating UI with
If I understood your concern correctly, the current |
… metadata to be a sub-component - Replace metadata prop with Page.TitleMetaData sub-component - Split actions into positional slots (PrimarySlot, SecondarySlot, TertiarySlot) and opinionated buttons (PrimaryAction, SecondaryAction) - Page.Menu no longer owns its positional wrapper, goes inside TertiarySlot - Remove typeof children === "string" checks from Subtitle and Intro; default styling always applies, fixing i18n/Trans compatibility - Remove hasCustomChildren type guard and discriminated union (no longer needed)
…ns, Intro and subtitle
…acy to composable migration steps
|
the only thing I'm wondering about is the naming. we haven't used the term "slot" before, and where I think it makes things a bit less obvious is what to put where. so while we want to offer flexibility, there is still a standard structure that we want to not necessarily enforce, but at least guide people towards. to me that seems valid. nothing in here indicates this is even remotely undesirable. vs it feels a bit weird to be putting a Combobox inside a component named "Menu" and it at least guides towards or communicates that yes we still have some opinions on what a Page contains. you're welcome to modify it but this is what it should be for consistency. so instead what I would propose is: I do think it can be a bit unclear where to me an "action" is a Page concept, whereas as Button is a literal element - so while there is a bit of redundancy between Page.ActionPrimary -> Page.PrimaryButton, in my head that makes sense and is conceptually coherent. also I realize that there's some asymmetry here. the first two action containers are more generic, and only the last one says "Menu". I think that might be ok, while we should use Buttons for the first two - it's really the last one, the Menu, that we probably have the strongest opinion on. this is where overflow actions go that weren't the primary nor secondary. I think putting some light JSDocs on the ActionPrimary and ActionSecondary saying "these should be Buttons" could suffice. what do you think about that? does this naming make sense to you? |
- Replace PrimarySlot, SecondarySlot, and TertiarySlot with ActionPrimary, ActionSecondary, and ActionMenu. - Update action components to use PrimaryButton and SecondaryButton for better consistency. - Modify related tests and documentation to reflect the new structure.
@ZakaryH , I think this naming convention is reasonable. It might be somewhat inconsistent with the legacy API which uses |
| let metaDataElement: ReactNode = null; | ||
| const otherChildren: ReactNode[] = []; | ||
|
|
||
| Children.forEach(children, child => { |
There was a problem hiding this comment.
ah I missed this earlier. with very few exceptions, we should avoid iterating over the children prop. it's a strong signal/smell that we've missed a step while constructing our component or API.
rather than having Page.Title accept two things, let's make each part so simple.
function PageTitleBar({ children, ...rest }: { readonly children: ReactNode }) {
const dataAttrs = filterDataAttributes(rest);
return (
<div className={styles.titleRow} {...dataAttrs}>
{children}
</div>
);
}
/** Renders the page heading (H1). */
function PageTitle({ children, ...rest }: PageTitleProps) {
// this probably doesn't work yet but it's not advertised so that might be fine
const dataAttrs = filterDataAttributes(rest);
return (
<Heading level={1} {...dataAttrs}>
{children}
</Heading>
);
}
/** Metadata displayed alongside the page title (e.g. status badges). Use inside Page.Title. */
function PageTitleMetaData({ children }: PageTitleMetaDataProps) {
return <>{children}</>;
}
now, similar to the actions and whatnot - if someone really really wants to have a h2 in the title, they can do
<Page.TitleBar>
<Heading level={2}>...</Heading>
</Page.TitleBar>
they shouldn't do that, but that's part of the deal we are making now. more power, and more responsibility.
the PageTitleMetaData sticks out to me now. it's literally nothing. we have no opinion of what it should or could be. it's just some arbitrary element that is a sibling to the Heading/Title element.
so now I question if it's even worth having? what would be the downside of
<Page.TitleBar>
<Page.Title>Hello World</Page.Title>
<StatusLabel label="In Progress" alignment="start" status="warning" />
</Page.TitleBar>
to me that seems fine. the whole TitleMetaData concept is too nebulous. it lacks any kind of definitive shape, and because of that I would say it's not the responsibility or part of the Page's identity.
so with that in mind, what if we instead go for...
<Page.Title>
<Page.TitleLabel>
<Trans>Hello World</Trans>
<Page.TitleLabel>
<StatusLabel label="In Progress" alignment="start" status="warning" />
</Page.Title>
I don't feel too strongly about any of the names. Page.Title as the container is OK. not my favorite, and the Page.TitleLabel also feels meh.
Page.TitleBar or Page.TitleRow are similar. I like Row less because visually, the Actions are on that row too and in my mind I think of a row as spanning the entire width.
so Page.TitleBar + Page.Title might be my preference but I'll leave it up to to you to decide.
really, the big things I want us to do here are:
- avoid iterating over children
- simplify building blocks as much as we can within reason
- discard the concept of TitleMetaData. it can now be totally delegated outside the component, and achieved with composition
this will mean a small additional example or note about how to implement a TitleMetaData in the new version
There was a problem hiding this comment.
I believe I addressed all of your points in e0c23d8.
| let actionsElement: ReactNode = null; | ||
| const otherChildren: ReactNode[] = []; | ||
|
|
||
| Children.forEach(children, child => { |
There was a problem hiding this comment.
same feedback here. I would love to avoid iterating over children. lemme think about how to split this up...
There was a problem hiding this comment.
so one simple answer is to split it up more and add another layer. Page.HeaderContent.
/** Wraps the title area (title, subtitle). Renders in the left slot of the header layout. */
function PageHeaderContent({ children }: { readonly children: ReactNode }) {
return <div>{children}</div>;
}
which allows PageHeader to be purely layout, no logic or children iteration
function PageHeader({ children, ...rest }: PageHeaderProps) {
const dataAttrs = filterDataAttributes(rest);
return (
<Content>
<Container
name="page-titlebar"
autoWidth
dataAttributes={dataAttrs as CommonAtlantisProps["dataAttributes"]}
>
<Container.Apply autoWidth>
<div className={styles.titleBar}>
{children}
</div>
</Container.Apply>
</Container>
</Content>
);
}
and finally used like
<Page.Header>
<Page.HeaderContent>
<Page.TitleBar>
<Page.Title>Kitchen Renovation Project</Page.Title>
<StatusLabel label="In Progress" ... />
</Page.TitleBar>
<Page.Subtitle>Everything but the Kitchen Sink</Page.Subtitle>
</Page.HeaderContent>
<Page.Actions>
...
</Page.Actions>
</Page.Header>
the downside is that we're getting so deeply nested with all this and it's really tough to know what goes where. we are going to rely heavily on examples and documentation, and perhaps trial-and-error. having Header > HeaderContent > TitleBar > Title, then siblings of Subtitle, and Actions is a lot..... part of me wonders if we create a named CSS grid, if that makes it simpler. you don't have to worry about so much nesting, we give you one container and then know where to put everything. it might save a layer or two of nesting.
also I do feel pretty weird about PageHeaderContent being a single unclassed div though. it makes me wonder what it is there for and if we can't solve that some other way...
|
seeing an issue on small screens 
vs master 
this might be related to another request we got recently about full width menu buttons |
| /* Composable Page API: responsive layout via CSS Container Queries */ | ||
|
|
||
| /* Base composable behavior (always active inside Container) */ | ||
| @container page-titlebar (min-width: 0) { |
There was a problem hiding this comment.
if we want something to always be there, or be treated as the default - we can simply define these as the style, no need for a greater than 0 width container query
.titleBar > .actionGroup {
display: flex;
flex-wrap: wrap;
justify-content: space-between;
}
There was a problem hiding this comment.
couple questions about this container query approach:
didn't we have to revert it last time? what was the issue?
I don't see a container-type definition, does this work without one? I would have expected to see container-type: inline-size on the overall Page container element.
does this now mean we are managing the layout swaps with 2 mechanisms in the same component? one with the titleBarRef and one with these container queries?
lastly, I wonder if a CSS grid could both solve the layout, assuming we aren't reintroducing whatever issue we had to revert for, AND if it would remove at least one layer of component nesting + make it easier to use as far as putting things in the exact expected order.
There was a problem hiding this comment.
basically what I'm thinking is this
Page Header is where we have the vast majority of our elements. we know we want at least 2 things:
- a layout that responds to the container size
- minimal layers of components
- ideally, an easy to use API/structure
using a CSS grid, we can label each area and tell it where to go based on the container query. this solves the layout, AND it makes it so that it is order independent. meaning if I do
<Page.Header>
<Page.Title>...</Page.Title>
<Page.Actions>...</Page.Actions>
<Page.Subtitle>...</Page.Subtitle>
</Page.Header
it yields the same thing as
<Page.Header>
<Page.Title>...</Page.Title>
<Page.Subtitle>...</Page.Subtitle>
<Page.Actions>...</Page.Actions>
</Page.Header
and makes it just a bit easier to use
then it might also remove a layer but I need to double check that. in my head it seems to me we have one other layer here that is trying to achieve this same result that we wouldn't need any more..
| }); | ||
|
|
||
| return ( | ||
| <Content> |
There was a problem hiding this comment.
do we need this?
also, now I see how this works with the container queries. it's the Container element below.
while it is nice to use our components, the concern I have is that we are solving the same problem in two different ways inside the same component, the titleBarRef and container queries.
what I would be tempted to do instead is set the container-type on .page so both can use that method, and then have our queries based on that.
is there a reason you'd like to do it with 2 different mechanisms for each version?
|
I had a few different comments, so just to summarize in rough priority order container queries the props driven Page and the composed Page now handle the same problem, container sensitive layout adjustments, in 2 distinct ways. the old one with the I worry that this can introduce drift in experience, and we'll have 2 different implementations to fix and maintain. if the container query works, I feel we should apply that to both - however that's not necessarily the goal of this PR so sticking to the old way may be preferable. child iteration as a consequence of the above - on its own, TitleMetaData becomes shapeless and arguably not a built in part of Page because we seem to have no opinions on what it should be. a webstory example showing how you could do it would suffice. small screen layout issue CSS Grid extra Content |
… API Replace implicit child iteration with explicit wrapper components: - PageTitle: remove TitleMetaData detection loop; introduce Page.TitleBar as an explicit flex container for title + metadata siblings - PageHeader: remove Actions detection loop; introduce Page.HeaderContent to group title area content, and remove redundant Content wrapper Co-authored-by: Cursor <cursoragent@cursor.com>
* instead of Container (container queries) * this is temporary, and we'll switch both legacy and composable Page to use container queries shortly (hopefully)
ZakaryH
left a comment
ZakaryH
left a comment
There was a problem hiding this comment.
LGTM
thanks for all your hard work on this PR and for putting up with all my comments!
4 participants
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
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.