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

Navigation Menu Button Example: fix unreachable code and shift-tab not closing menu #716

Merged
merged 2 commits into from Jun 27, 2018
Merged

Navigation Menu Button Example: fix unreachable code and shift-tab not closing menu #716

merged 2 commits into from Jun 27, 2018

Conversation

jongund
Copy link
Contributor

@jongund jongund commented Jun 15, 2018

Issue #396: Fixes the unreachable code problem
Issue #588: Fixes shift-tab not closing the menu
Updated CSS focus styling

Preview link

Navigation menu button example in Jon's fork

@mcking65 mcking65 changed the title fixed unereachable code problem and shift-tab problem Navigation Menu Button Example: fix unreachable code and shift-tab not closing menu Jun 24, 2018
@mcking65
Copy link
Contributor

@jongund, the fix for issue 588 looks good. I think 396 as well; @sh0ji, could you look at that?

I'm curious about the css changes. They were not part of either of these issues. Please describe them.

@sh0ji
Copy link
Contributor

sh0ji commented Jun 24, 2018

The 396 fix looks good to me.

The style changes look purely cosmetic to me, but there are at least two issues:

  • The border of the role=menu is not connecting at the top-left and top-right corners the way it should.
  • The role=menuitem boxes resize on hover a tiny bit, causing the text to shift.

I'd recommend doing the style changes in a different PR if that's possible.

@mcking65
Copy link
Contributor

@sh0ji, thank you! @jongund, I too would prefer a separate PR for the styling. In general, I prefer one PR per issue ... it doesn't have to always be that way for small issues, but it is a good rule of thumb.

Please back out the CSS file changes and I will merge.

Then, if you want to address Evan's feedback in another PR, that would be good.

@jongund
Copy link
Contributor Author

jongund commented Jun 25, 2018

I removed the CSS changes from the original pull request

@mcking65 mcking65 merged commit 8bc433e into w3c:master Jun 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants