Skip to content
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

[Issue 704] Breadcrumbs #776

Merged
merged 12 commits into from
Nov 30, 2023
Merged

[Issue 704] Breadcrumbs #776

merged 12 commits into from
Nov 30, 2023

Conversation

SammySteiner
Copy link
Contributor

Summary

Fixes #704

Time to review: 10 mins

Changes proposed

Added breadcrumb component, constants, types, story, and test. Added component to process and research page.

Additional information

Process:
image

Research:
image

Story:
image

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we infer hierarchy from routes so we don't have to define the breadcrumb trail for each page? Might there be downsides for doing that? We'd still need a lookup for the title strings. 🤔 Just thinking out loud here. No change required, maybe just food for thought as we get into more complex info architecture and page/route structures.

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 thought about that too and was considering using https://nextjs.org/docs/app/api-reference/functions/use-pathname to get and and then break up the path. But realized as you mentioned we'd still need to look up the title strings based on the path, so doesn't really save us much and can be harder to track down if it breaks.

};
export default meta;

export const Home = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we'd ever show breadcrumbs on root. Okay to show how it'd work if we did tho.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I wasn't sure what to use for the default story. I could change it to be either Research or Process 🤷, let me know if you have a preference

Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably fine as-is. We can revisit in the future as our route/page requirements become more complex.

Copy link
Collaborator

@acouch acouch left a comment

Choose a reason for hiding this comment

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

LGTM!

@SammySteiner SammySteiner merged commit fbc50d9 into main Nov 30, 2023
8 checks passed
@SammySteiner SammySteiner deleted the sammysteiner/704-breadcrumbs branch November 30, 2023 14:02
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.

[Task]: Add breadcrumbs component for subpages
3 participants