feat(design): Add truncated design to DaffBreadcrumbComponent#4278
feat(design): Add truncated design to DaffBreadcrumbComponent#4278joannalauu wants to merge 17 commits intodevelopfrom
Conversation
|
Blocked by #3998 (needs to be merged before this) |
| <ng-template #fullMenu> | ||
| <daff-menu> | ||
| @for (breadcrumbItem of _fullMenuItems(); track breadcrumbItem; let index = $index) { | ||
| <button daff-menu-item> |
There was a problem hiding this comment.
I have not been able to find a good solution to achieve this without breaking other behaviour.
Right now, the breadcrumb is passed into the menu through
<ng-container [ngTemplateOutlet]="breadcrumbItem.itemRef"></ng-container>
as <a href="/docs">Docs</a>, and there isn't really a way to extract the href and innerHTML while keeping the menu content up-to-date, because the menu is usually not re-rendered when switched to another page.
Perhaps it might be better to just keep the structure as it is, even through it isn't ideal? Please let me know if you have any ideas on an effective solution to this.
1f98924 to
147afb4
Compare
4b6e3f4 to
5704534
Compare
|
@xelaint I really sorry about not thoroughly review my code before requesting a review. I have fixed the breadcrumb behaviour to work as intended, aside from the daff-menu structure which I have elaborated more in the discussion thread. |
|
We need to share the interface between MenuItem and BreadcrumbItem so that the actual HTML element is an a or button in both circumstances. We don't want buttons inside buttons or a in a, or new a where it could have been the original a element from the breadcrumb. |
5d55ea5 to
d0ef854
Compare
xelaint
left a comment
There was a problem hiding this comment.
active is still not set on the active breadcrumb item
58e0277 to
ce007e8
Compare
| } | ||
| } | ||
|
|
||
| &:nth-child(2), |
There was a problem hiding this comment.
Can the logic for calculating what is shown be set by toRenderType instead of having to do it via css?
| expect(breadcrumbItems.length).toBe(4); | ||
| }); | ||
|
|
||
| it('should compute 5 breadcrumb items', () => { |
There was a problem hiding this comment.
Why would 5 breadcrumb items be computed if there are less than 5 breadcrumbs? Shouldn't the max be 4?
| DaffBreadcrumbItemComponent, | ||
| ], | ||
| }) | ||
| class WrapperComponentWith4Items {} |
There was a problem hiding this comment.
Please avoid using digits in class names
libs/design/breadcrumb/README.md
Outdated
| ### Daffodil provides | ||
| - `aria-current="page"` automatically applied to the last breadcrumb item | ||
| - Enforces semantic HTML structure (requires `<ol>` and `<li>` elements) | ||
| - Truncates breadcrumbs into an overflow menu on mobile and when more than five items are present on desktop |
There was a problem hiding this comment.
Doesn't belong in accessibility. Should be moved into a ## Features section after ## Anatomy:
## Features
### Truncation
Breadcrumbs are automatically truncated into an overflow menu on mobile viewports. On desktop, truncation occurs when more than five items are present.
ce007e8 to
16591ec
Compare
16591ec to
2d78c82
Compare
| fixture7.detectChanges(); | ||
| }); | ||
|
|
||
| it('should compute 5 breadcrumb items', () => { |
There was a problem hiding this comment.
this test is confusing. if there are more than 5 breadcrumb items, it should compute more than just 5.
| } | ||
| } | ||
|
|
||
| <ng-template #fullMenu> |
There was a problem hiding this comment.
@damienwebdev thoughts on updating this template name to #mobileMenu and the #partialMenu as #desktopMenu? I think this is more semantically correct since these menus are rendered depending on what screen size the display is.


This also deprecates the DaffMenuModule.
PR Checklist
PR Type
Current behavior
Fixes: #3513
Developed on top of #3569
New behavior
Breaking change?
Additional context