-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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(editor): Implement breadcrumbs component #13317
Conversation
Codecov ReportAttention: Patch coverage is 📢 Thoughts on this report? Let us know! |
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.
This is a really cool component, love the flexibility 🙌 Left some questions
packages/design-system/src/components/N8nBreadcrumbs/Breadcrumbs.vue
Outdated
Show resolved
Hide resolved
packages/design-system/src/components/N8nBreadcrumbs/Breadcrumbs.vue
Outdated
Show resolved
Hide resolved
packages/design-system/src/components/N8nBreadcrumbs/Breadcrumbs.vue
Outdated
Show resolved
Hide resolved
packages/design-system/src/components/N8nBreadcrumbs/Breadcrumbs.vue
Outdated
Show resolved
Hide resolved
packages/design-system/src/components/N8nBreadcrumbs/BreadCrumbs.test.ts
Outdated
Show resolved
Hide resolved
packages/design-system/src/components/N8nBreadcrumbs/Breadcrumbs.vue
Outdated
Show resolved
Hide resolved
packages/design-system/src/components/N8nBreadcrumbs/Breadcrumbs.vue
Outdated
Show resolved
Hide resolved
packages/design-system/src/components/N8nBreadcrumbs/Breadcrumbs.vue
Outdated
Show resolved
Hide resolved
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.
Happy to ship as is 🚀
packages/design-system/src/components/N8nActionToggle/ActionToggle.vue
Outdated
Show resolved
Hide resolved
@@ -33,7 +33,7 @@ const emit = defineEmits<{ | |||
}>(); | |||
const props = withDefaults(defineProps<Props>(), { | |||
hiddenItems: () => [], | |||
hiddenItems: () => new Array<PathItem>(), |
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.
Oh sorry, I was just asking if this should be hiddenItems: []
, but I guess this might get us a reference to the same object every time like in Python parameter defaults 😅
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.
Ah, we must use the function. In Vue, Object or array defaults must be returned from a factory function like this. I just though types array like this is cleaner
n8n
|
Project |
n8n
|
Branch Review |
ADO-3176-breadcrumbs-component
|
Run status |
|
Run duration | 04m 45s |
Commit |
|
Committer | Milorad Filipovic |
View all properties for this run ↗︎ |
Test results | |
---|---|
|
0
|
|
1
|
|
5
|
|
0
|
|
436
|
View all changes introduced in this branch ↗︎ |
✅ All Cypress E2E specs passed |
|
1 similar comment
|
✅ All Cypress E2E specs passed |
Co-authored-by: Rob Squires <robtf9@icloud.com>
Summary
Breadcrumbs component that will be used in upcoming folders feature. Should support two verisons (
small
andmedium
) and hidden path items should support async loading.More details are in Figma linked to the Linear issue.
Related Linear tickets, Github issues, and Community forum posts
Closes ADO-3176
Review / Merge checklist
release/backport
(if the PR is an urgent fix that needs to be backported)