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

A11Y improvements (especially to the Dutch texts) #69

Merged
merged 6 commits into from
Jul 19, 2023

Conversation

dirkinfi
Copy link
Contributor

Lijndikte aangepast voor zichtbaarheid
Afstand tot de tekst wat groter gemaakt voor leesbaarheid

@netlify
Copy link

netlify bot commented Jun 23, 2023

Deploy Preview for the-infi-way ready!

Name Link
🔨 Latest commit fbfa80d
🔍 Latest deploy log https://app.netlify.com/sites/the-infi-way/deploys/64b82e169eb7010008d61188
😎 Deploy Preview https://deploy-preview-69--the-infi-way.netlify.app/nl
📱 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 configuration.

@dirkinfi dirkinfi changed the title Improved underline A11y changes Dirk Jun 23, 2023
@dirkinfi
Copy link
Contributor Author

LET OP: ik heb overal <span lang="en"> en toegevoegd, maar de resultaten zijn nog niet helemaal eenduidig. Ik ga er maandag met Sterre naar kijken.

@jeroenheijmans
Copy link
Member

jeroenheijmans commented Jun 26, 2023

Cheers! I had the same question @dirkinfi - does this add a significant improvement? I'd presume that screen readers are very capable of dealing with loan words, and that they'd ignore short snippets in another lang?

PS. There's an open discussion in #63 whether we want to go back with this entire open source project to Dutch as the default language, but if we could for now stick to English conversations that would be appreciated 🙏🏻

@jeroenheijmans jeroenheijmans self-requested a review June 26, 2023 09:33
@jeroenheijmans jeroenheijmans marked this pull request as draft June 26, 2023 09:34
@dirkinfi
Copy link
Contributor Author

@jeroenheijmans screen readers do NOT handle loanwords gracefully (you'll get horrible steenkolenengels), and it's not a big amount of work to add the language-tag. That said, if found out that VoiceOver (the Apple default screenreader) does not always handle the tagged snippets correctly either. So as a learning experience, Sterre is asking Job if this is something we can improve or not. But adding the correct tags is in my opinion a small task, and you should (imho) get into the habit of adding them.

@dirkinfi
Copy link
Contributor Author

(BTW I'm new to interfering with code and working in Github, so feel free to talk to me if I do this 'wrong'. For example, I have no idea what the desired opknipstrategie is for pull requests. Personally I'd separate the line-thickness fix from the language-tag-fix, but Steven recommended putting them in one branch 'a11y changes'.)

@jeroenheijmans
Copy link
Member

Thx @dirkinfi for going along with the GitHub process! So far so good, and many choices (like how granular things should be) are also subjective, so asking around is often a good solution.

Let's discuss the status of the PR in person some time soon, and then we'll bring back our conclusions to GitHub for visibility? We can probably finish this work together and get the improvements merged.

@jeroenheijmans
Copy link
Member

Update: Discussed the PR offline with Dirk, will summarize here a bit later and help get the changes across. We also find several other improvements (some A11Y related) which I'll put into fresh tickets.

dirkinfi and others added 5 commits July 9, 2023 22:56
Lijndikte aangepast voor zichtbaarheid
Afstand tot de tekst wat groter gemaakt voor leesbaarheid
Zodat ie door een screenreader met het juiste accent wordt uitgesproken
Zodat ie door een screenreader met het juiste accent wordt uitgesproken
Zodat de screenreader al onze engelse termen goed uitspreekt
Discussed offline with Dirk, the originator of this PR: wherever
possible it's best if the Dutch text doesn't use loan words, for
sure for Screen Readers. When it's not possible to use Dutch, we
can use spans to switch the narrator to English pronunciation.
@jeroenheijmans jeroenheijmans marked this pull request as ready for review July 9, 2023 22:20
@jeroenheijmans jeroenheijmans changed the title A11y changes Dirk A11Y improvements (especially to the Dutch texts) Jul 9, 2023
@jeroenheijmans
Copy link
Member

After some discussion and investigation together with @dirkinfi we found that:

  • reducing English loan words in the Dutch texts is preferred whenever possible, the screen reader sticks to a single voice in one fluent reading
  • some Dutch loan words get proper pronunciation if the spelling is tweaked, e.g. "backups" does not work well, but "back-ups" is read properly (at the least by Narrator)
  • abbreviations don't always need to be marked English, and the proper semantic html <abbr> can fix screen reading
  • we still doubt in some cases if it's "worth" having a <span lang="en"> term halfway through a sentence, because it causes a pause, and possibly screen reader users are actually accustomed to loan words not causing such a switch in voice... still for many cases it sounded best to leave those span elements there so I left them

Additionally:

  • I reviewed all changes carefully again
  • I rebased and fixed all merge conflicts (because of recent other merges)
  • I marked this PR as ready for review
  • I will add the other improvements Dirk and I discussed in separate issues

I'll ask a second person to double check the end result, but I think it's now good to go.

Copy link
Member

@jeroenheijmans jeroenheijmans left a comment

Choose a reason for hiding this comment

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

LGTM now. Will ask for a second pair of eyes to check it later this week.

@jeroenheijmans jeroenheijmans merged commit b18bac4 into main Jul 19, 2023
4 checks passed
@jeroenheijmans jeroenheijmans deleted the dirkinfi-a11y-changes branch July 19, 2023 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants