-
Notifications
You must be signed in to change notification settings - Fork 30
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
feat(components): add breadcrumb to Page
#597
Conversation
✔️ Deploy Preview for jobber-atlantis ready! 🔨 Explore the source changes: 001b57d 🔍 Inspect the deploy log: https://app.netlify.com/sites/jobber-atlantis/deploys/60fb428520f2c3000713e207 😎 Browse the preview: https://deploy-preview-597--jobber-atlantis.netlify.app |
This reverts commit 2899ee1.
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.
Overall looking good! Added comments which are suggestions and not necessarily must be addressed IMO
{breadcrumb && ( | ||
<Button | ||
icon="backArrow" | ||
url="../" |
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 added flexibility, would we be able to have 2 additional properties of a breadcrumb?
interface breadcrumb {
label: string;
/* Default = "../" */
url?: string;
onClick?(): void;
}
If the onClick
property is passed, the url is ignored
{breadcrumb && ( | ||
<Button | ||
icon="backArrow" | ||
url="../" | ||
label={breadcrumb} | ||
type="tertiary" | ||
size="small" | ||
></Button> | ||
)} |
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 multi-level breadcrumbs were to be a possibility, we could make it so that the breadcrumb type is an array, if the length is 1, use the button style and then some other type of style for multiple?
/**
* Label for the "back" button when Page is nested.
* The breadcrumb button follows a relative URL to jump up one level.
*/
readonly breadcrumbs?: Breadcrumb[];
interface Breadcrumb {
label: string;
/* Default = "../" */
url?: string;
onClick?(): void;
}
👌👀👌👀👌👀👌👀👌👀 good shit go౦ԁ sHit👌 thats ✔ some good👌👌shit right👌👌there👌👌👌 right✔there ✔✔if i do ƽaү so my self 💯 i say so 💯 thats what im talking about right there right there (chorus: ʳᶦᵍʰᵗ ᵗʰᵉʳᵉ) mMMLGTMMᎷМ💯 👌👌 👌НO0ОଠOOOOOОଠଠOoooᵒᵒᵒᵒᵒᵒᵒᵒᵒ👌 👌👌 👌 💯 👌 👀 👀 👀 👌👌Good shit |
Needs more discussion |
@chris-at-jobber, can we create an issue to capture that discussion? Having the breadcrumb functionality would be great, and be better to have a defined pattern instead of using buttons |
Motivations
A few one-off instances of "back button" breadcrumbs have been used alongside
Page
- a good indicator that it's time to incorporate this mechanism into Page.Changes
Adds a breadcrumb to Page that will jump up one level in the application's navigation directory.
Added
breadcrumb
property that sets the breadcrumb label and determines if it existsIn Atlantis we use Github's built in pull request reviews.