Conversation
Aligns Breadcrumbs component to Fusion DS. Updates styles using fusion-tokens and adds new `inverse` prop. Deprecates `isDarkBackground` prop from Breadcrumbs and `hasFocus`, `underline`, `linkSize` and `bold` props from Crumb.
edleeks87
left a comment
edleeks87
left a comment
There was a problem hiding this comment.
Nice work @nuria1110, some minor suggestions from me
| { children, isDarkBackground = false, inverse, ...rest }: BreadcrumbsProps, | ||
| ref, | ||
| ) => { | ||
| const l = useLocale(); |
There was a problem hiding this comment.
| const l = useLocale(); | |
| const locale = useLocale(); |
not related to your changes but I think it's more readable to use the full word here
|
|
||
| interface DividerProps { | ||
| isDarkBackground: boolean; | ||
| inverse?: boolean; |
There was a problem hiding this comment.
| inverse?: boolean; | |
| $inverse?: boolean; |
we should prepend any non-valid attributes with $ to ensure they're not add to the rendered DOM element
| @@ -3,24 +3,24 @@ import Link, { LinkProps } from "../../link"; | |||
|
|
|||
| interface StyleCrumbProps extends LinkProps { | |||
| isCurrent?: boolean; | |||
There was a problem hiding this comment.
| isCurrent?: boolean; | |
| $isCurrent?: boolean; |
| interface StyleCrumbProps extends LinkProps { | ||
| isCurrent?: boolean; | ||
| inverse: boolean; | ||
| inverse?: boolean; |
There was a problem hiding this comment.
| inverse?: boolean; | |
| $inverse?: boolean; |
|
|
||
| export interface CrumbProps | ||
| extends Omit< | ||
| extends Pick< |
| const { isDarkBackground } = useBreadcrumbsContext(); | ||
| const { inverse } = useBreadcrumbsContext(); | ||
|
|
||
| if (rest.hasFocus && !deprecatedHasFocusWarn) { |
There was a problem hiding this comment.
comment: I think as these props weren't likely to have been used simply annotating the interface will be enough
| isCurrent={isCurrent} | ||
| aria-current={isCurrent ? "page" : undefined} | ||
| inverse={isDarkBackground} | ||
| inverse={inverse} |
There was a problem hiding this comment.
| inverse={inverse} | |
| $inverse={inverse} |
| <li> | ||
| <StyledCrumb | ||
| ref={ref} | ||
| isCurrent={isCurrent} |
There was a problem hiding this comment.
| isCurrent={isCurrent} | |
| $isCurrent={isCurrent} |
| margin-left: var(--global-space-comp-s); | ||
| font: var(--global-font-static-comp-regular-m); | ||
|
|
||
| ${({ inverse }) => css` |
There was a problem hiding this comment.
| ${({ inverse }) => css` | |
| ${({ $inverse }) => css` |
| expect(onClick).toHaveBeenCalledTimes(0); | ||
| }); | ||
|
|
||
| test("applies aria-current attribute when isCurrent is true", () => { |
There was a problem hiding this comment.
nitpick: Such a minor pathetic thing to point out but it's just to keep the formatting the same as the other added tests.
| test("applies aria-current attribute when isCurrent is true", () => { | |
| test("applies aria-current attribute when 'isCurrent' is true", () => { |
3 participants
Proposed behaviour
Aligns
Breadcrumbscomponent to Fusion DS.:inverseprop.isDarkBackgroundprop from Breadcrumbs.hasFocus,underline,linkSizeandboldprops from Crumb.Current behaviour
Breadcrumbscomponent is not aligned with Fusion DS.Checklist
d.tsfile added or updated if requiredQA
Additional context
There is a bug in
react-docsgen-typescriptwhere the props table will not generate in storybook when a variable is declared outside of the scope of a component exported as default, so I've had to add a named export forCrumbto fix this.Testing instructions