feat(chrome): add new CollapsibleSubNavItem component #187
Conversation
austingreendev
requested review from
allisonacs,
jordanalviso,
jzempel,
rauschermate and
ryanseddon
|
|
||
| const COMPONENT_ID = 'chrome.collapsable_sub_nav_item'; | ||
| const PANEL_COMPONENT_ID = 'chrome.collapsable_sub_nav_item_panel'; | ||
| const SUB_NAV_ITEM_HEIGHT_PX = 38; |
There was a problem hiding this comment.
Item height is variable – can wrap to multiple lines. I was hoping this measurement could be obtained and applied post-render.
There was a problem hiding this comment.
If we are animating the max-height over time, how would we calculate that total height without having a render flash? Lets discuss offline.
There was a problem hiding this comment.
On post-render, iterate over the children and get element.offsetHeight + top-margin + bottom-margin for each. Apply the value to the panel as style="max-height: [value]".
There was a problem hiding this comment.
I just pushed an update that adds a new AccordionContainer component. This abstracts all of the accessibility logic required for the W3C accordion pattern
I also calculated the panel maxHeight using a slightly different approach. Lets discuss offline.
|
|
||
| componentDidUpdate() { | ||
| if (this.props.expanded && this.panelRef) { | ||
| this.panelRef.style.maxHeight = `${this.panelRef.offsetHeight}px`; |
There was a problem hiding this comment.
This doesn't appear to be animating at all. You can slow down the panel expansion by adding transition: max-height 2s ease-in-out !important; to the styled component to ensure you've got it working.
jzempel
left a comment
jzempel
left a comment
There was a problem hiding this comment.
Yessss! This is looking good. Please just remove the @zendeskgarden/svg-icons duplicate from "dependencies" and we should be good to go!
|
|
||
| getHeadingProps = ({ role = 'heading', ...other } = {}) => { | ||
| return { | ||
| role, |
There was a problem hiding this comment.
@Austin94 per https://www.w3.org/TR/wai-aria-practices/#accordion, the role="heading" also needs a value for aria-level. Can this be addressed so that the consumer can feed in that value (and in the demo so that it passes the axe audit)?
There was a problem hiding this comment.
Voiceover was smart enough to auto-detect the level, didn't realize that AXE was complaining.
This is now a required attribute and prop for the AccordionContainer and the CollapsibleSubNavItem.
Looks like I erroneously added it as a dependency when we first created the package. Now removed. |
🚧 WIP, still trying to get animation working correctly and some API tweaks 🚧
Description
This PR adds a new
CollapsibleSubNavItemcomponent.Detail
Checklist
designer as a reviewer)
component
yarn start)src/index.jsexport