-
Notifications
You must be signed in to change notification settings - Fork 2k
Dashboard v2: implement breadcrumbs #103294
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~14219 bytes removed 📉 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~3467 bytes added 📈 [gzipped])
Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Async-loaded Components (~1565 bytes added 📈 [gzipped])
React components that are loaded lazily, when a certain part of UI is displayed for the first time. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
loader: async ( { params: { siteSlug } } ) => { | ||
await queryClient.ensureQueryData( siteSettingsQuery( siteSlug ) ); | ||
return { | ||
breadcrumbItemLabel: __( 'Settings' ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this have to be in the loader? Can't it be added as route meta or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It needs to call __()
, so it can't be a static data. Also, I imagine other screens might have dynamic breadcrumb item label such as plugin name in the Plugins dashboard (it's the case now in v1 if you go to https://wordpress.com/plugins)
BTW as per docs, meta
has been replaced with https://tanstack.com/router/latest/docs/framework/react/guide/static-route-data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is my conclusion after spending a day reading the docs and consulting with AI 🥲 but I'm open if you have better ideas! This is literally my first project with TanStack stack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
__()
can be called anywhere, it doesn't have to be called at render time.
But if you really want, you could create a function that maps a route to a label:
function getRouteBreadcrumbLabel( route ) {
switch ( route.path ) {
case 'setting': return __( 'Settings' );
}
}
The loader function doesn't seem meant for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you point me to the designs for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The design is not available yet in v2 actually. I just wanted to prepare for this case in case we want to bring this pattern to v2 😬
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think yeah we can try to put that in staticData
(formerly meta
) first and try to figure out the dynamic case when it's actually needed in the design...
__() can be called anywhere, it doesn't have to be called at render time.
I was afraid the translation might not be ready yet when we construct the routing, but I'm happy to be wrong. I can try.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, imo we shouldn't think about use cases that might not exist. If for now a simple link with a slash works, then imo that's good for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was afraid the translation might not be ready yet when we construct the routing, but I'm happy to be wrong. I can try.
For @wordpress/i18n
in GB, I don't think it matters, I'm not sure about Calypso. But if you want to be sure it could be a function that maps it instead that is called during render
'!@automattic/components/src/core-badge', | ||
'!@automattic/components/src/breadcrumbs', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clicking the breadcrumb items will trigger a page reload. This is because it's a regular link, not a router link. I don't have any good idea yet how to solve it. Need help from @Automattic/io.
How difficult is it to just create the breadcrumb links ourselves? In this case, it looks like there will only ever be a single breadcrumb link, looking at the designs, so this doesn't seem that complex to add a single {link} /
above the header?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I honestly initially thought about that... but then why are we creating a full-blown <Breadcrumbs />
DS component if it's never going to be used beyond the above simple case? That thinking made me end up trying to come up with a more general breadcrumbs system 🥲
Even if we have to hardcode the router link, we will need to copy-paste the styles from the <Breadcrumbs />
component. Not sure if that's a good idea design-wise 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, if we're only ever going to add one link then we can remove that component eventually. I don't think we should necessarily import styles from Calypso components, we should focus on using @wordpress
styles with limited customisation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, this is not what I anticipated; I thought the idea is to make use of the reusable DS components. I'll see what I can do next week; it's getting late for me 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or I can try to pass something to the link's onClick
props and see if I can properly call TanStack router navigation with it.
<a href={ href } onClick={ onClick } className="a8c-components-breadcrumbs__item"> |
I'd love to get input from @ntsekouras as the component author 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently Breadcrumbs was created for the hosting dashboard, but I don't have more information than that, so ignore my comments for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or I can try to pass something to the link's onClick props and see if I can properly call TanStack router navigation with it.
For start that what I'd try to do personally, just for now. Or it might had to do with the path (relative/full, etc..).
Breadcrumbs
will be part of the PageHeader component, that will land soon. My plan was to implement what this PR does, after the merge of PageHeader and try to see if we can make it work with a
tags, but if it can't, we'll add support in the Breadcrumbs component to pass the element we want instead of a
( in this case Link
). This was mentioned in the Breadcrumbs PR already.
So, for me the important part here is to create the functionality to retrieve the proper breadcrumb items and for now we could just use directly a Link and not the Breadcrumbs if not possible.
Part of DOTCOM-13067
Proposed Changes
This PR implements breadcrumbs and show it on the child pages of Settings as per design (p1746709722501659/1746709518.587029-slack-C08JZTRS6NA)
Implementation details
breadcrumbItemLabel
prop to the route's context. Any route can append this prop by returning this in itsloader()
.-siteSettingsSubscriptionGiftingRoute
(and future setting routes) as a child ofsiteSettingsRoute
.siteSettingsRoute
.<PageLayout />
.Open discussions
<PageLayout />
toapp/
notcomponents/
? Because it is not a "pure" component anymore; it contains logic (breadcrumbs).Testing instructions
/v2/sites
Pre-merge Checklist