[Breadcrumbs]: Created breadcrumbs component #167
Conversation
|
We'll get back to you regarding a CLA as well, which we need to get sorting before we are able to merge code. 👍 |
| [routerLinkUrl]="link.url" | ||
| *ngIf="link.url" | ||
| [linkTitle]="link.label"></fudis-link> | ||
| <span *ngIf="!last" class="fudis-separator">/</span> |
There was a problem hiding this comment.
The current design for breadcrumb component is as follows:
Change the separator span to fudis-icon with chevron.
| align-items: center; | ||
| } | ||
|
|
||
| .fudis-separator { |
There was a problem hiding this comment.
Remove unnecessary class due to span --> fudis-icon change.
If the class is still needed, it should follow BEM, .fudis-breadcrumbs__separator
| @@ -0,0 +1,16 @@ | |||
| @use '../../foundations/colors/mixins.scss' as colors; | |||
| @use '../../foundations/spacing/tokens.scss'; | |||
There was a problem hiding this comment.
Follow the same convention in all @use cases, add as spacing;
| @@ -0,0 +1,11 @@ | |||
| <nav aria-label="Breadcrumb navigation" class="fudis-breadcrumbs" *ngIf="links.length > 0"> | |||
| <ng-container *ngFor="let link of links; let last = last"> | |||
| <fudis-link | |||
There was a problem hiding this comment.
Fix the semantic structure, links should be wrapped inside list
<ul><li><fudis-link /></li></ul>
Also, current page should be communicated for screen readers with aria-current="page".
| [color]="last ? 'default' : 'primary'" | ||
| [routerLinkUrl]="link.url" | ||
| *ngIf="link.url" | ||
| [linkTitle]="link.label"></fudis-link> |
There was a problem hiding this comment.
Let's use self-closing tags for fudis-link.
| $duration-long1: 450ms; | ||
| $duration-long2: 500ms; | ||
| $duration-long3: 550ms; | ||
| $duration-long4: 600ms; |
There was a problem hiding this comment.
Nice addition for motion tokens but for now let's not clutter the codebase until these have gone through our designers. Leave just that one token which is in use in icon component.
Change the naming of the token for something more intuitive, e.g $duration-600
ngx-fudis/projects/ngx-fudis/src/lib/components/breadcrumbs/breadcrumbs.component.ts
Show resolved
Hide resolved
| styleUrls: ['./breadcrumbs.component.scss'], | ||
| }) | ||
| export class BreadcrumbsComponent { | ||
| @Input() links: { label: string; url: string }[] = []; |
There was a problem hiding this comment.
Separate and make new type FudisBreadcrumb to types/miscellaneous.ts and use it here.
| @@ -0,0 +1,11 @@ | |||
| <nav aria-label="Breadcrumb navigation" class="fudis-breadcrumbs" *ngIf="links.length > 0"> | |||
There was a problem hiding this comment.
Change this aria-label to be required Input instead of hard-coded value.
We want to be able to modify the label in order to tell the screen reader which breadcrumb is being navigated e.g in different sections of the page and that the breadcrumbs are in correct user language.
|
Thanks for the feedback, will fix. |
|
Sorry about the delay, we're working on the CLA and hopefully it's ready soon. |
|
recheck |
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
Sorry for the delay @tommi-miettinen, our CLA system is now ready if you're still willing to resolve the remarks in the review we can get this merged. :) |
|
@tommi-miettinen: Are you still interested in this PR? We need to move forward with a breadcrumb implementation and we'd rather not implement one from scratch if you're still willing to improve on this (or sign our CLA from above and we'll continue on from this PR). |
|
@Tsallanmaa Sorry i had forgotten about this, i will do it during the weekend. |
|
I have read the CLA Document and I hereby sign the CLA |
|
Let's merge as is and fix tests and other necessary stuff in other PR |
RiinaKuu
changed the title
Hey, fixed the suggested changes with linting.
The breadcrumbs component needed a way to render last link as disabled so i added a disabled class to the link component.
I added motion tokens from the google material design 3 guidelines
https://m3.material.io/styles/motion/easing-and-duration/tokens-specs
Saw that expandable components closing doesnt have animation so i added a rotation reset class to icon.