feat: add new breadcrumbs component
#215
Conversation
|
Y'all reviewers. The "next" svg icons look low to me. I wonder if we need to vertical center |
|
@jzempel good point, they do look low; I don't think it's about descenders. |
There was a problem hiding this comment.
@jzempel yea, and the react somehow looks different than the CSS.
CSS:
React:
Whatever is causing it, the chevrons need to be better vertically centered against the text.
Edit: Just confirmed in Sketch, the chevrons are placed a pixel lower in the react component. The screen grab makes it looks like 2px; this is bc it's a retina screenshot:
|
|
||
| script: | ||
| - yarn lint | ||
| - yarn format |
There was a problem hiding this comment.
@jzempel What prompted this change?
I would be concerned if Travis was testing and deploying code that was transformed by a format without it being visible in Github.
There was a problem hiding this comment.
We seem to continue to let non-formatted code slip through (misconfigured husky?). The travis build will fail if the git tree isn't clean after test.
There was a problem hiding this comment.
Oh that's perfect 💯
|
@allisonacs @ginnywood the demo page has been updated with the latest CSS code: https://garden.zendesk.com/react-components/breadcrumbs/ |
…rk structure and pass axe testing
| getContainerProps = ({ role = 'navigation', ...other } = {}) => { | ||
| return { | ||
| role, | ||
| 'aria-label': 'Breadcrumb navigation', |
There was a problem hiding this comment.
Should we default this like role in the argument destructuring to make it more obvious to teams that they'll need to translate this? Should we mention in the readme example that we hard code an english value?
There was a problem hiding this comment.
Should we default this like role in the argument destructuring to make it more obvious to teams that they'll need to translate this?
Not sure how clean this would be since we would be destructuring a hyphenated value?
Should we mention in the readme example that we hard code an english value?
Definitely.
There was a problem hiding this comment.
@ryanseddon I added copy to the element example about expected override. But I don't think I can move 'aria-label' into the destructured parameter list as it's an invalid property name.
Description
Adds a new
@zendeskgarden/react-breadcrumbscomponent package.Detail
Demo pre-published for review at https://garden.zendesk.com/react-components/breadcrumbs/
/cc @Austin94 😉
Checklist
designer as a reviewer)
component
yarn start)src/index.jsexport