-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(SideNav): add screen size check for aria-hidden value #10087
fix(SideNav): add screen size check for aria-hidden value #10087
Conversation
✔️ Deploy Preview for carbon-react-next ready! 🔨 Explore the source changes: dff5dce 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-react-next/deploys/61f21a7397420b00075df9e6 😎 Browse the preview: https://deploy-preview-10087--carbon-react-next.netlify.app/ |
✔️ Deploy Preview for carbon-elements ready! 🔨 Explore the source changes: dff5dce 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/61f21a735623c2000750fb69 😎 Browse the preview: https://deploy-preview-10087--carbon-elements.netlify.app |
✔️ Deploy Preview for carbon-components-react ready! 🔨 Explore the source changes: dff5dce 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/61f21a73c20bd8000861fcc1 😎 Browse the preview: https://deploy-preview-10087--carbon-components-react.netlify.app/ |
@dakahn lmk when this is ready for review :) |
Co-authored-by: Josh Black <josh@josh.black>
Co-authored-by: Josh Black <josh@josh.black>
Co-authored-by: Josh Black <josh@josh.black>
@joshblack pushed up some tests for that hook. I'm at a total loss for your second testing comment. Wouldn't testing the state is in sync be testing the onChange was calling properly for the hook? also it seems like eslint jest have renamed some rules in a recent update. I'm getting this error when I commit Which corresponds with this rule -- that it doesn't seem like i'm breaking, but I could just be missing something. |
@dakahn let me know if you need help with the tests! Totally free this afternoon |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Just a small commented out console left :)
Co-authored-by: Abbey Hart <abbeyhrt@gmail.com>
Closes #9895
The sidenav on screens greater than 1056px default to open (with the menu button hidden). The default expanded state is bugged (potentially in HeaderContainer which seems to expect isSideNavExpanded as a prop?). This adds a check that basically account for a screen width of larger than 1056 where the side nav is opened and not aria-hidden.
I consider this a stopgap while functional component refactors are happening to the UIShell and it's components. 🏄🏾
🎆 🎆 🎆 NOW FEATURING A USEMATCHMEDIA HOOK 🎆🎆 🎆
Testing / Reviewing
check that the aria hidden value on the
element inside header is properly set to false when sidenav is visible and true when it isn't 👍🏾