Skip to content

Conversation

@ktbee
Copy link

@ktbee ktbee commented Oct 27, 2019

Hello! Thank you for making such a nice free theme available. I noticed there were a couple of small fixes I could make to improve accessibility and keyboard navigation. My proposed changes:

  • Re-add default focus styling to the navbar toggle button so keyboard users can see when they've tabbed to the menu button on smaller screens. It looks like bootstrap.min.css had disabled it with outline:0.
  • Move CSS for showing nav link children out of a media query so that it affects desktop nav links as well. This is because when I tabbed to the dropdown nav link, I couldn't open the drop down by hitting the enter key.
  • Update JS comment to reflect new behavior.

Thanks again and looking forward to your thoughts on this!

@daattali
Copy link
Owner

Thanks for the great PR Katie (and also for your breakup advice site - "If you loved the wrong person this much, think how much you can love the right person", good service to a lot of people!)

Do you have your proposed changes implemented in a demo website where I can see exactly what it would look like?

By just looking at the code, I'm 100% happy with the second change that makes menus tab-able, neat. The first change because it would affect everyone, I'd want to see what it looks like. I certainly wasn't thinking about this sort of accessibility when I agreed to remove the hover effect on the menu button #256 (comment)

@ktbee
Copy link
Author

ktbee commented Nov 5, 2019

Ha I'm glad you like the Breakup Survival Guide site @daattali! My friend @emilytheis had the inspiration and did a lot of the front end for that site :)

As for the outline styling, I can understand your hesitation to add that for all your users. I ran the site locally using jekyll serve, so I don't have a live version. If it helps though, I can set one up on Github pages. Also a less risky change would be to set this outline styling on just the focus state and not hover. I'm picturing something like:

.navbar-default button.navbar-toggle:focus {
  outline: 1px dotted #212121;
  outline: 5px auto -webkit-focus-ring-color;
}

Let me know if you prefer just the focus styling, and I'll set up a live version of my changes as well. Thanks!

@daattali
Copy link
Owner

daattali commented Nov 5, 2019

Yes please do set up a fork with your proposed changes. And yes on focus only sounds less intrusive

@daattali
Copy link
Owner

daattali commented May 2, 2020

Hi @ktbee , I just did a complete rewrite of the theme and updated all the CSS to help accessibility. If you still want to help with this PR, I'd be happy to accept it when it's ready!

Copy link

@lucalves lucalves left a comment

Choose a reason for hiding this comment

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

Conflict in the files, please resolve.

@daattali
Copy link
Owner

daattali commented Sep 4, 2020

Closing this PR due to inactivity. If anyone wants to revive it in the future, feel free to submit a new one!

@daattali daattali closed this Sep 4, 2020
adam-abed-abud pushed a commit to adam-abed-abud/old-adam-abed-abud.github.io that referenced this pull request Oct 30, 2024
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.

3 participants