Skip to content

Bring back topbar breadcrumbs #2529

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

Merged
merged 29 commits into from
Nov 10, 2024
Merged

Bring back topbar breadcrumbs #2529

merged 29 commits into from
Nov 10, 2024

Conversation

charliepark
Copy link
Contributor

This PR switches from the current topbar navigation — using dropdowns that are a little too clever — and moves to a more standard breadcrumb navigation.

Screenshot 2024-10-30 at 10 41 01 AM Screenshot 2024-10-30 at 10 41 13 AM

I expect there'll be a few tweaks to make re: spacing / sizing, and making the element responsive / more intelligent about narrow viewports.

Closes #2509

Copy link

vercel bot commented Oct 30, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
console ✅ Ready (Inspect) Visit Preview Nov 10, 2024 8:27pm

@david-crespo
Copy link
Collaborator

I think this looks great. Huge improvement. A couple of issues to work out:

No project detail page

This has always been a little weird, but this brings it out. Because there is nothing interesting about a project besides its name and description, we have always used the instance list as a project homepage. But that means both the project link and the instances link go to the same thing. There are some possible workarounds, but it might be easiest to add a project detail page. On the other hand such a page would probably not be that useful.

2024-10-30-project-breadcrumb.mp4

Back arrow when there's nothing to go back to

Just a little weird, but I'm not sure what else we would do. Also it's slightly weird for Projects to be the only white thing on the page.

image

It's less weird when there are other elements in the breadcrumbs because the point is the contrast, though maybe still slightly weird when you think about it. Though as you can see here we have a couple of other white elements here and there, like the section headings. Doesn't feel like there's a clear principle to it.

image

@charliepark
Copy link
Contributor Author

charliepark commented Oct 30, 2024

Have removed the arrow from single-level root pages, and also set the higher contrast link to only show up when there are multiple levels.
Screenshot 2024-10-30 at 3 16 54 PM
Screenshot 2024-10-30 at 3 19 43 PM

@benjaminleonard
Copy link
Contributor

Addressing some of the visual things now. As an aside, was looking into how a clickable back button might work...tricky and still not quite right.

  const matches = useMatches()
  // Finds the next available route to jump to
  // If there's no handle we have no breadcrumb
  // and skip: true means we skip that route
  const parentRoute =
    matches
      .filter(
        (match) => !!match.handle || (match.handle && !(match.handle as RouteHandle).skip)
      )
      .map((match) => match.pathname)
      .slice(0, -1)[0] || ''

You then add a skip to routes you don't want to go back to – in this case we'd skip over the instance page since we're on a tab route.

<Route
  element={<InstancePage />}
  loader={InstancePage.loader}
  handle={{ skip: true }}
>

I can get the parent route mostly okay, but the redirects make it a little funky. Must be a better way.

@@ -38,7 +38,7 @@ export function TopBar({ children }: { children: React.ReactNode }) {
{/* Height is governed by PageContainer grid */}
{/* shrink-0 is needed to prevent getting squished by body content */}
<div className="z-topBar border-b bg-default border-secondary">
<div className="mx-3 flex h-[60px] shrink-0 items-center justify-between">
<div className="mx-3 flex h-[54px] shrink-0 items-center justify-between">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was going to ask if it was worth making this a var, but that might only be worthwhile if we need to use it in more places.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WELP

<div className="!mx-0 flex h-full max-h-[calc(100vh-60px)] !w-full flex-col">

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

womp

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dropped this down from 60 to 54, but this is technically for delineating between the body and the footer. @benjaminleonard do we want the serial console footer to match the header, at 54px? Or to stay at 60?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in that case it's making the layout the full height of the screen, minus the size of the top bar. The bottom bar on the serial console seems a bit chunky but it's matching the instance create bottom bar. Might adjust elsewhere.

@benjaminleonard
Copy link
Contributor

On the high contrast thing – I've set it to text-secondary for now. Note that when I make the text-default token changes it'll be in more places and we can use it here without it looking unusual.

@david-crespo
Copy link
Collaborator

david-crespo commented Nov 1, 2024

This version also has a problem with routes nested under route tabs I noted in my refactor, though it it's a little different — only the routers tab gets a crumb.

2024-11-01-routers-crumb-2.mp4

* first pass at matches-based breadcrumbs. route config changes required

* kinda fix things in the route config

* use-title.ts -> use-crumbs.ts

* Update import

---------

Co-authored-by: Charlie Park <[email protected]>
@david-crespo
Copy link
Collaborator

david-crespo commented Nov 7, 2024

Here is me demoing approximately all the crumbs. They look pretty good except for the ones where there's a crumb that doesn't actually link to a page or that links to a route that redirects to the page you're already on, as @charliepark pointed out. (I also noticed issues on SSH key and floating IP pages, fixed in dfb3143).

2024-11-07-testing-out-crumbs.mp4

And here is me having some fun with the top bar height CSS var.

2024-11-07-top-bar-height-var.mp4

@david-crespo
Copy link
Collaborator

I experimented with a link: false annotation for the <Route>s that don't have their own pages, but it felt weird and bad not to be able to click them. I'm just going to try and make sure they at least don't flash weird blank pages on click.

@david-crespo
Copy link
Collaborator

One approach that would work for avoiding blank screen flashes on redirecting nodes is hacking in a way for the crumb for /projects/mock-project to actually point to /projects/mock-projects/instances. That would likely mean annotating the route handle with another field crumbPath that can override match.pathname as the link path for the crumb. This is pretty straightforward to do.

A more evil approach is to make the loaders for the /projects/mock-project route node match the loaders for /projects/mock-projects/instances so they are loaded before we hit /projects/mock-project and therefore we don't see a flash because the redirect is instantaneous. This works in my testing, but it's much uglier and more error-prone because it can easily become wrong if the loaders for the target route change.

-      <Route path="projects/:project" element={<Navigate to="instances" replace />} />
+      <Route
+        path="projects/:project"
+        loader={(args) =>
+          Promise.all([ProjectLayout.loader(args), InstancesPage.loader(args)])
+        }
+        element={<Navigate to="instances" replace />}
+      />

@@ -119,8 +115,6 @@ export const pb = {
ipPoolEdit: (params: IpPool) => `${pb.ipPool(params)}/edit`,
ipPoolRangeAdd: (params: IpPool) => `${pb.ipPool(params)}/ranges-add`,

inventory: () => '/system/inventory',
rackInventory: () => '/system/inventory/racks',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The deleted ones in here were never used. Some were used to construct other paths but never used directly.

@charliepark
Copy link
Contributor Author

This is all looking great.

funny how that works
@david-crespo david-crespo merged commit 9ef82ba into main Nov 10, 2024
8 checks passed
@david-crespo david-crespo deleted the breadcrumbs branch November 10, 2024 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TopBar UI is above SideModalForm overlay Make breadcrumbs more recognisable as breadcrumbs
3 participants