Skip to content

feat: add breadcrumbs to admin settings pages #15865

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 19 commits into from
Dec 18, 2024

Conversation

jaaydenh
Copy link
Contributor

@jaaydenh jaaydenh commented Dec 13, 2024

@jaaydenh jaaydenh self-assigned this Dec 13, 2024
@jaaydenh jaaydenh changed the base branch from main to jaaydenh/settings-sidebar-split December 13, 2024 21:39
Copy link
Collaborator

@BrunoQuaresma BrunoQuaresma left a comment

Choose a reason for hiding this comment

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

The code looks good, but the breadcrumbs are not aligned with either the navbar's left element or the sidebar items.

Screenshot 2024-12-16 at 06 00 39

Since it’s not a blocker, I’m approving it—even though you don’t need this, as you’re pointing the PR to another PR, not main 😆.

@jaaydenh
Copy link
Contributor Author

@BrunoQuaresma the reason the breadcrumbs are not aligned is because it is dependent on the changes to the topbar navigation, #15617

Also I wanted to get approval because I am treating it as a standalone PR to make review easier and its dependent on #15388

@jaaydenh jaaydenh requested a review from chrifro December 16, 2024 15:51
@jaaydenh jaaydenh force-pushed the jaaydenh/settings-sidebar-split branch 2 times, most recently from 1c6fe52 to ac71d6c Compare December 17, 2024 18:32
@jaaydenh jaaydenh force-pushed the jaaydenh/admin-settings-breadcrumbs branch from e01b74b to 5ede03c Compare December 17, 2024 18:39
<BreadcrumbItem>
<BreadcrumbPage>Admin Settings</BreadcrumbPage>
</BreadcrumbItem>
<BreadcrumbSeparator />
Copy link
Member

Choose a reason for hiding this comment

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

I feel like inserting the separator shouldn't be the responsibility of the consumer. The BreadcrumbList could use Children.toArray and intersperse them automatically, you could use some CSS tricks with :not(:first-of-kind) or whatever, etc. There are other options that reduce the risk of misusage.

Also why are <Breadcrumb> and <BreadcrumbList> even separate components?

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 was originally thinking that the Breadcrumb should be more universal so that it could be used in places without the separator line underneath and this is how shadcn/ui does it as well. Of course, if the argument is that breadcrumbs for Coder will always have a separator line underneath, it could make sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having both and is a common pattern in the shadcn/ui components to add some semantic meaning. In general, I am not focused on altering or adding opinions to the way the components are constructed but always open to suggestions.

Base automatically changed from jaaydenh/settings-sidebar-split to main December 18, 2024 22:18
@jaaydenh jaaydenh force-pushed the jaaydenh/admin-settings-breadcrumbs branch from 2e82bfb to 5b428e4 Compare December 18, 2024 22:22
@jaaydenh jaaydenh merged commit cdc1978 into main Dec 18, 2024
29 checks passed
@jaaydenh jaaydenh deleted the jaaydenh/admin-settings-breadcrumbs branch December 18, 2024 22:35
@github-actions github-actions bot locked and limited conversation to collaborators Dec 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

site: Add Breadcrumbs for admin settings pages
3 participants