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

[WIP] Feature/a11y improvements #182

Closed
wants to merge 14 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@unused
Contributor

unused commented Jul 16, 2018

Some easy improvements that may be helpful for many people. Please check the commits for details & descriptions. PR is wip but opened as any feedback is very welcome.

  • Set document language
  • Update alt tags for graphical images
  • Ensure all images have an alt tag
  • Ensure keyboard navigation (visual focus, tab order, labels)
  • Ensure links have proper descriptions
  • Suggest improvements for font handling (font-size is too small)
  • Suggest color contrast improvements
  • Ensure logical document structure, content order
  • Document findings in brief Frontend developers guidelines

unused added some commits Jul 16, 2018

Define language for the HTML document
Set the `lang` attribute in `html` tag in order to let an assistive
technology know what language the content is delivered. You don't want
to know how a french screen reader reads out the content of an English
website if this attribute is missing.
Set alt text for logo and nav-divider graphics
Note that an alternative text for images should always be present. It
represents a very brief textual description of the content of the image.
If an image is used for some graphical/design purpose only, it should
have an empty alt content. This will be not be read out by an assistive
device. If the alt attribute is missing, the image filename will be
read.
@iHiD

This comment has been minimized.

Contributor

iHiD commented Jul 16, 2018

This is brilliant. Thank you 💙

@iHiD iHiD changed the title from WIP: Feature/a11y improvements to [WIP] Feature/a11y improvements Jul 22, 2018

@unused

This comment has been minimized.

Contributor

unused commented Jul 25, 2018

@iHiD I guess you are using some database dump for development - if you don't mind I'd extend the db:seeds to have some dummy data? That would make it way easier for a local setup and frontend checks.

@iHiD

This comment has been minimized.

Contributor

iHiD commented Jul 26, 2018

@unused I don't mind. You should be able to get full track and exercise data from the git rake tasks if you're after that.

@iHiD

This comment has been minimized.

Contributor

iHiD commented Jul 26, 2018

This is all super amazing btw! Thank you 💙

@iHiD

This comment has been minimized.

Contributor

iHiD commented Jul 26, 2018

@unused Feel free to ping me on Slack if you have anything I can help with in terms of dummy data etc.

@@ -1,7 +1,8 @@
module ApplicationHelper
def code_person_widget
content_tag :div, id: "widget-code-person" do
image_tag random_person_image_url
image_tag random_person_image_url,
alt: 'Programming person with notebook'

This comment has been minimized.

@iHiD

iHiD Jul 26, 2018

Contributor

Could we change notebook to laptop here pls, as I would hear notebook to be a physical notebook (maybe an i8n difference?)

This comment has been minimized.

@iHiD

iHiD Jul 26, 2018

Contributor

Or even maybe "computer" as moving forward with might not always have laptops.

A question (for my edification): Is there a "correct" version of "Person programming on a computer" vs "Programming person with computer". ie, specifying a noun vs verb/action. I see a thing happening when I see those images, rather than a static image. But I have no idea what's correct or helpful when writing alt tags.

This comment has been minimized.

@unused

unused Jul 26, 2018

Contributor

Thanks for the note, English is not my mother tongue so I'm happy to get feedback on mistakes. For sure I'll change it to computer.

There're a few things to deal with when describing an image:

  • Worst case is to skip the alt attribute, an assistive technology like a screen-reader will then probably read out the filename :(
  • If the purpose of an image is only to support design, use an empty value alt="", it will be ignored. Don't use aria here, it's meant to be used for special cases and it's most of the time better to use native HTML.
  • Briefly describe images to transport a similar information than you do visually. There's no need to go into details.
  • If the image holds some important content, like a chart does, you should try to describe it with more details. Though there are other strategies to use in such cases like creating interactive SVGs that support focusing on elements and description references, or simply adding visually hidden tables.
options[:onerror] = "this.onerror=null;this.src='https://assets.exercism.io/tracks/default-#{type}-#{color}.png'"
image_tag(track.send("#{type}_#{color}_icon_url"), options)
image_tag track.send("#{type}_#{color}_icon_url"), options

This comment has been minimized.

@iHiD

iHiD Jul 26, 2018

Contributor

Could we stick to using brackets in methods in helpers pls? That will keep it consistent with the rest of the codebase. I'm aware it's a personal preference, but I prefer the explicity of it. In the HAML, on the other hand, I prefer not.

At some point soon when I get chance, I will write these "style" things up as part of the contributing guide to this repo, and would also really appreciate your input on adding rules for how to keep things accessible beyond this initial PR.

This comment has been minimized.

@unused

unused Jul 26, 2018

Contributor

Sure, I couldn't figure out the style preferences for the project that's why I probably mixed it up a bit. Brackets for methods, space after equal sign in HAML templates =image or = image, single or double quotes in templates and code. Can you please give me very brief instructions?

This comment has been minimized.

@iHiD

iHiD Jul 26, 2018

Contributor

I will write a quick document with some notes in for this and then you can PR instructions for the a11y to it too :)

Ensure default button styles display focus state
Many browsers have default styles to show focus of an interactive
element like a button, input or link visually using the CSS attribute
outline. You can adapt or remove this for sure, but have to take care
the state can still be recognized.
@iHiD

This comment has been minimized.

Contributor

iHiD commented Jul 26, 2018

@unused What do you think about me merging this in its current state so you don't have to keep rebasing etc? If you then give me any guidelines on what to do moving forward, I can ensure we keep up to date. We're about to do a lot of work on the responsive side of the site, so I could see things becoming quite merge-conflict-y if we have one giant PR :)

Then you can continue with your next steps in a new PR (or a new PR for each step?)

@unused

This comment has been minimized.

Contributor

unused commented Jul 26, 2018

Sure we can merge this - but I'm afraid I could not manually test every change yet. Should I create a new PR with squashed commits for a clean history?

@iHiD

This comment has been minimized.

Contributor

iHiD commented Jul 26, 2018

I'm just on my way out now, but I'll test and merge this tomorrow morning my time :)

@unused

This comment has been minimized.

Contributor

unused commented Jul 26, 2018

Perfect, thanks a lot 👋

Fix typo in solution reflection view
vim quick-fix ^^ 🙈
@iHiD

This comment has been minimized.

Contributor

iHiD commented Jul 27, 2018

@unused

  • I've merged the first batch of this PR here Thank you! I'm leaving this PR open as a global tracking PR. You might want to just restart your branch from master now? Alternatively, you might want to create an issue on exercism/exercism.io for the tracking, and just do PRs for each step. I'll merge them the same day in all likelyhood.
  • I've created a contributing.md - please feel free to PR that lots as well.

Thanks again for doing this. It means a lot to me that you are :)

@unused

This comment has been minimized.

Contributor

unused commented Jul 29, 2018

Thank you for the update - I'll close this pull request and come up with small ones for the next changes.

@unused unused closed this Jul 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment