search

phillycommunitywireless / phillycommunitywireless

Website for the Philly Community Wireless Project
https://phillycommunitywireless.org
7 stars 5 forks source link

42: focus indicators #70

Closed qbatten closed 1 year ago

qbatten commented 1 year ago

Did a few things here:

  1. Created 2 different styles for :focus'ed elements. They use a light gray background, black font, and a black border. This results in higher contrast, the buttons/links standing out from the others without looking too out of place with the theme, and is also in keeping with normal expectations about what a focused element should look like. I'm absolutely not a graphic designer so totally open to changes to how they look.
  2. Split the custom.css file into a few different files (colors, fonts, etc). I kept "custom.css" as a sort of catch-all for other random bits of css that didn't fit into a clear category.
  3. I removed the !important property from the colors (in colors.css).
    • I did this bc that property was preventing me from changing the colors of things on :focus.
    • I did a manual check that this didn't actually change any of the colors rendered and I didn't notice any changes.
  4. I stripped out some styles that were written in-line in HTML and pulled them into custom.css (the actual styles are unchanged)
  5. Added tabindex="0" to places that seemed appropriate. That attr is what causes an element to be tab-selectable.

When reviewing the PR, it may be helpful to look at the first commit first— that commit shows all the changes to custom.css before I split it out into different files, which is easier to parse/understand than the overall PR diff.

qbatten commented 1 year ago

Video of tabbing thru the homepage on the site that's live right now: https://user-images.githubusercontent.com/9345870/186574208-cd973dd8-c836-4861-bce9-d4152b3a4c25.mp4

Video of tabbing thru the homepage on the site after this PR: https://user-images.githubusercontent.com/9345870/186574094-2e6819e7-6039-4b95-b286-8b8338ce629e.mp4

hawc2 commented 1 year ago

This looks great @qbatten! Thanks for this pull request. @addiebarron or @dzygmundfelt, would you have a moment this week to review and approve this pull request?

addiebarron commented 1 year ago

This all looks great! My only question is about the tabindex stuff. I looked into it a bit and it seems like elements which are natively focusable / interactive (a, input, textarea e.g.) don't need any explicitly declared tabindex in order to be accessible (reference from the a11yproject). It sounds like we'd be alright just leaving those elements as is. Let me know what you think.

qbatten commented 1 year ago

Ohh gotcha. That makes sense, I'll remove those.

qbatten commented 1 year ago

Done! I double checked and yeah none of those were needed lol, I just misunderstood how tab-navigation/tabindex works.

hawc2 commented 1 year ago

any one know why this pull request is failing? the error brings up alot of general complaints about it being a github issue. @dzygmundfelt cleaned up our deployment process just now (pull request #73), that might help this

dzygmundfelt commented 1 year ago

Looks like it's failing on the deploy step due to a permissions issue, which has nothing to do with the changes in this MR. As long as @qbatten made the changes that @addiebarron was asking for, it's fine to merge in.