-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
import { Breadcrumbs as BreadcrumbsComponent } from '@automattic/components/src/breadcrumbs'; | ||
import { useMatches } from '@tanstack/react-router'; | ||
import type { BreadcrumbItemProps } from '@automattic/components/src/breadcrumbs/types'; | ||
|
||
export default function Breadcrumbs() { | ||
const matches = useMatches(); | ||
ntsekouras marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
const items = [] as BreadcrumbItemProps[]; | ||
|
||
let hasParent = false; | ||
matches.forEach( ( match ) => { | ||
const breadcrumbItemLabel = match.staticData?.breadcrumbItemLabel; | ||
if ( breadcrumbItemLabel || hasParent ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd expect extracting the breadcrumbs based on how we register the routes (route.addChildren). Isn't that possible? It seems weird to have a |
||
items.push( { | ||
label: breadcrumbItemLabel?.() || '', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It wouldn't make any sense to have an empty label, so we could move this check before pushing the item. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was a "hack" because the individual settings subpage needs to be in the breadcrumb items; even if it's not shown (because it's the last item). Let me think on how to make it cleaner... |
||
href: match.pathname, | ||
} ); | ||
hasParent = true; | ||
} | ||
} ); | ||
|
||
return <BreadcrumbsComponent items={ items } />; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ import { | |
redirect, | ||
createLazyRoute, | ||
} from '@tanstack/react-router'; | ||
import { __ } from '@wordpress/i18n'; | ||
import { fetchTwoStep } from '../data'; | ||
import NotFound from './404'; | ||
import UnknownError from './500'; | ||
|
@@ -109,6 +110,9 @@ const siteSettingsRoute = createRoute( { | |
path: 'settings', | ||
loader: ( { params: { siteSlug } } ) => | ||
queryClient.ensureQueryData( siteSettingsQuery( siteSlug ) ), | ||
staticData: { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't have the full picture yet regarding the proper implementation, but here we decouple the route breadcrumb label with the label we use for the menus in the app. Would it be possible and/or preferable to have a single label in the route and extract this in the menus? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is already the case right now, we're duplicating the page title (e.g. |
||
breadcrumbItemLabel: () => __( 'Settings' ), | ||
}, | ||
} ).lazy( () => | ||
import( '../sites/settings' ).then( ( d ) => | ||
createLazyRoute( 'site-settings' )( { | ||
|
@@ -118,10 +122,8 @@ const siteSettingsRoute = createRoute( { | |
); | ||
|
||
const siteSettingsSubscriptionGiftingRoute = createRoute( { | ||
getParentRoute: () => siteRoute, | ||
path: 'settings/subscription-gifting', | ||
loader: ( { params: { siteSlug } } ) => | ||
queryClient.ensureQueryData( siteSettingsQuery( siteSlug ) ), | ||
getParentRoute: () => siteSettingsRoute, | ||
path: 'subscription-gifting', | ||
} ).lazy( () => | ||
import( '../sites/settings-subscription-gifting' ).then( ( d ) => | ||
createLazyRoute( 'site-settings-subscription-gifting' )( { | ||
|
@@ -292,8 +294,7 @@ const createRouteTree = ( config: AppConfig ) => { | |
siteOverviewRoute, | ||
siteDeploymentsRoute, | ||
sitePerformanceRoute, | ||
siteSettingsRoute, | ||
siteSettingsSubscriptionGiftingRoute, | ||
siteSettingsRoute.addChildren( [ siteSettingsSubscriptionGiftingRoute ] ), | ||
] ) | ||
); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
import '@tanstack/react-router'; | ||
|
||
declare module '@tanstack/react-router' { | ||
interface StaticDataRouteOption { | ||
breadcrumbItemLabel?: () => string; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,24 +1,33 @@ | ||||||||||||||||||||||||||
import { useQuery } from '@tanstack/react-query'; | ||||||||||||||||||||||||||
import { Outlet, useMatch, useMatches } from '@tanstack/react-router'; | ||||||||||||||||||||||||||
import { | ||||||||||||||||||||||||||
__experimentalHeading as Heading, | ||||||||||||||||||||||||||
__experimentalVStack as VStack, | ||||||||||||||||||||||||||
Card, | ||||||||||||||||||||||||||
} from '@wordpress/components'; | ||||||||||||||||||||||||||
import { __ } from '@wordpress/i18n'; | ||||||||||||||||||||||||||
import { siteQuery, siteSettingsQuery } from '../../app/queries'; | ||||||||||||||||||||||||||
import { siteRoute } from '../../app/router'; | ||||||||||||||||||||||||||
import { siteSettingsRoute } from '../../app/router'; | ||||||||||||||||||||||||||
import PageLayout from '../../components/page-layout'; | ||||||||||||||||||||||||||
import SubscriptionGiftingSettingsSummary from '../settings-subscription-gifting/summary'; | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
export default function SiteSettings() { | ||||||||||||||||||||||||||
const { siteSlug } = siteRoute.useParams(); | ||||||||||||||||||||||||||
const { siteSlug } = siteSettingsRoute.useParams(); | ||||||||||||||||||||||||||
const { data: siteData } = useQuery( siteQuery( siteSlug ) ); | ||||||||||||||||||||||||||
const { data: settings } = useQuery( siteSettingsQuery( siteSlug ) ); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
const matches = useMatches(); | ||||||||||||||||||||||||||
const match = useMatch( { from: siteSettingsRoute.id } ); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
if ( ! siteData || ! settings ) { | ||||||||||||||||||||||||||
return null; | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
const isExactMatch = match.id === matches[ matches.length - 1 ].id; | ||||||||||||||||||||||||||
if ( ! isExactMatch ) { | ||||||||||||||||||||||||||
return <Outlet />; | ||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand why There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I want the subpage routes (in this case
So, if it's an exact match, we want to render the Settings page, otherwise, we should render There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
On the other hands, we cant really compose Settings and its subpages. In |
||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
return ( | ||||||||||||||||||||||||||
<PageLayout title={ __( 'Settings' ) } size="small"> | ||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this be the same label as the breadcrumb? Could it be more generally the route label? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually yes. @ntsekouras also had a similar comment in that it should also match with the nav menus ( wp-calypso/client/dashboard/sites/site-menu/index.tsx Lines 7 to 18 in c320da3
If we want to use route label, given we want to put the labels in the router level as proposed in this PR, this will then become something like <PageLayout title={siteSettingsRoute.options.staticData.breadcrumbItemLabel()} /> but I'm not sure if it's a good pattern in TanStack. Another way is maybe write some hook called In any case, it's a good follow-up thing to do, whatever approach we take! |
||||||||||||||||||||||||||
<Heading>{ __( 'General' ) }</Heading> | ||||||||||||||||||||||||||
|
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.
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.wp-calypso/packages/components/src/breadcrumbs/index.tsx
Line 45 in bb42801
I'd love to get input from @ntsekouras as the component author 🙏
Uh oh!
There was an error while loading. Please reload this page.
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.
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 witha
tags, but if it can't, we'll add support in the Breadcrumbs component to pass the element we want instead ofa
( in this caseLink
). 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.
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 opened a PR for this: #103321
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.
If you do want to shortcut #103321 you can implement the
onClick
handler likeThere 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.
@p-jackson The downside of this approach is that it doesn't support "preloading". See #103283 and same issue also being solved for DataViews #103328
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.
Thanks all. I understand that we're discussing how we will support route links for the breadcrumbs in the linked PR above. To keep progressing, can we then move forward with the existing Breadcrumbs component behavior in this PR, and then we can follow-up later when we have decided how to support route links? At least, in this PR, we managed to collect the breadcrumbs and show them in the appropriate location.