Skip to content
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

13417 menu loss of focus #13716

Closed

Conversation

guidari
Copy link
Contributor

@guidari guidari commented May 2, 2023

Closes #13417
Closes #13415

I added a new prop to act like a function, so when the focus leaves the SideNav this fucntion will trigger and it will switch the expanded prop in the HeaderContainer

Testing / Reviewing

Switch to mobile view. Open the SideNav by click on the hamburger menu, then enter and leave the SideNav to test the behaviour

@guidari guidari requested review from a team as code owners May 2, 2023 19:23
@netlify
Copy link

netlify bot commented May 2, 2023

Deploy Preview for carbon-components-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 9eae89f
🔍 Latest deploy log https://app.netlify.com/sites/carbon-components-react/deploys/645e6f52a4d91d00084a3158
😎 Deploy Preview https://deploy-preview-13716--carbon-components-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented May 2, 2023

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit 9eae89f
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/645e6f520f75f1000884b1be
😎 Deploy Preview https://deploy-preview-13716--carbon-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks pretty good, and in most cases the menu closes as expected!

I was able to find this case though where the menu does not close as expected.

I click to put focus on the canvas, and then move focus through the menu. I get to the end of the menu, it does not close, and continue moving focus again with tab. You can see the button being focused behind the menu and the page scroll location moving as focus moves between the button and link on the page.

menu.does.not.close.and.focus.of.items.on.page.is.obscured.mov

@guidari
Copy link
Contributor Author

guidari commented May 9, 2023

Fix! I forgot to add the new prop to the stories you were testing. That's why didn't work there

@guidari guidari requested a review from tay1orjones May 9, 2023 16:49
Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

Copy link
Collaborator

@francinelucca francinelucca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM! I see how this solves for #13417 , but is this also solving for #13415 ? I don't see a difference in the content being obscured 🤔
image

@guidari
Copy link
Contributor Author

guidari commented May 9, 2023

About the #13415

When the focus leaves the SideNav and goes to the main content the screen still obscure.
So basically this fix solve that issue also, since the SideNav is collapsed and the light effect goes away

I was talking with Taylor about that in the morning. Let me know if that make sense

Screenshot 2023-05-09 at 11 37 38

@guidari guidari requested a review from francinelucca May 9, 2023 18:56
Copy link
Collaborator

@francinelucca francinelucca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for walking me through that @guidari !

@guidari guidari force-pushed the 13417-menu-loss-of-focus branch 4 times, most recently from 9cac047 to 45a6893 Compare May 12, 2023 14:40
@francinelucca francinelucca changed the base branch from main to alisonjoseph-patch-1 May 12, 2023 16:42
@francinelucca francinelucca requested review from laurenmrice and a team as code owners May 12, 2023 16:42
@francinelucca francinelucca changed the base branch from alisonjoseph-patch-1 to main May 12, 2023 16:42
@guidari guidari force-pushed the 13417-menu-loss-of-focus branch 2 times, most recently from 66dddcd to 9eae89f Compare May 12, 2023 16:54
@guidari guidari closed this May 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants