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

Adds aria-label to all nav items to make them accessible. #1516

Merged

Conversation

@jaimeiniesta jaimeiniesta mentioned this pull request Mar 18, 2020
37 of 37 tasks complete
@@ -23,7 +23,7 @@
{% import "partials/language.html" as lang with context %}

<!-- Table of contents -->
<nav class="md-nav md-nav--secondary">
<nav class="md-nav md-nav--secondary" aria-label="Table of contents">

This comment has been minimized.

Copy link
@squidfunk

squidfunk Mar 18, 2020

Owner

Please use the translation "toc.title" here. See the base template for how to use translations.

@squidfunk

This comment has been minimized.

Copy link
Owner

squidfunk commented Mar 18, 2020

Hmm, only adding English titles is not that great actually. How are the aria-label titles used? Do screen readers read them aloud?

@jaimeiniesta

This comment has been minimized.

Copy link
Author

jaimeiniesta commented Mar 18, 2020

Hmm, only adding English titles is not that great actually. How are the aria-label titles used? Do screen readers read them aloud?

I think that's the idea yes, these labels can be read aloud or be converted into a structured list to aid with navigation maps. So if there were a single nav in the page there is no problem, but if there are many, each of them needs to be uniquely labelled so they can be properly described. Instead of "navigation, navigation, navigation", the user will hear "navigation: main, navigation: footer, etc.".

So yes these should be translated.

@squidfunk

This comment has been minimized.

Copy link
Owner

squidfunk commented Mar 18, 2020

okay then, could you add the translations to src/partials/language/en.html then? This is the fallback for when no other translations are given.

We can call the keys:

  • header.title
  • footer.title
  • nav.title
  • tabs.title
@jaimeiniesta

This comment has been minimized.

Copy link
Author

jaimeiniesta commented Mar 18, 2020

Done! I've translated all the aria-labels - the new ones and an existing one for the search placeholder.

I've also included Spanish translations for the new keys.

Copy link
Owner

squidfunk left a comment

Could you create a PR for the refactor/rxjs-typescript branch? As v5 is on the horizon I suggest we leave master as-is.

@@ -27,8 +27,10 @@
"clipboard.copy": "Copy to clipboard",
"clipboard.copied": "Copied to clipboard",
"edit.link.title": "Edit this page",
"footer.title": "Footer",

This comment has been minimized.

Copy link
@squidfunk

squidfunk Mar 18, 2020

Owner

Minor issue: please ensure that keys are sorted alphabetically.

This comment has been minimized.

Copy link
@jaimeiniesta

jaimeiniesta Mar 18, 2020

Author

Alright, I'll fix the rest of the existing keys also as they were not in order.

This comment has been minimized.

Copy link
@squidfunk

squidfunk Mar 18, 2020

Owner

Ah wait. Okay, alphabetical order makes no sense, as language and directionality should be at the top. Nevermind, then.

@@ -26,8 +26,10 @@
"clipboard.copy": "Copiar al portapapeles",
"clipboard.copied": "Copiado al portapapeles",
"edit.link.title": "Editar esta página",
"footer.title": "Pie",

This comment has been minimized.

Copy link
@squidfunk

squidfunk Mar 18, 2020

Owner

Same issue (alphabetical ordering)

@@ -27,7 +27,7 @@
<label class="md-search__overlay" for="__search"></label>
<div class="md-search__inner" role="search">
<form class="md-search__form" name="search">
<input type="text" class="md-search__input" aria-label="search" name="query"
<input type="text" class="md-search__input" aria-label="{{ lang.t('search.placeholder') }}" name="query"

This comment has been minimized.

Copy link
@squidfunk

squidfunk Mar 18, 2020

Owner

Why do we need an aria-label when there's already a placeholder set?

This comment has been minimized.

Copy link
@squidfunk

squidfunk Mar 18, 2020

Owner

Ah, sorry, haven't seen that it was already there. Somebody added it at some point. Can we remove this?

This comment has been minimized.

Copy link
@jaimeiniesta

jaimeiniesta Mar 18, 2020

Author

I don't think we need it, I was just using the translation here (this aria-label was there before I came). 😄

I'll remove that one.

@squidfunk

This comment has been minimized.

Copy link
Owner

squidfunk commented Mar 18, 2020

I just did a search and there're more aria-label entries, like for example reset for the search reset button. We should also provide translations for that. So to sum up, I think we should do the following things:

  • PR to refactoring branch
  • Order labels alphabetically
  • Check if we need aria-label if there's a placeholder set
  • Add translation for the search reset button
@jaimeiniesta

This comment has been minimized.

Copy link
Author

jaimeiniesta commented Mar 18, 2020

  • PR to refactoring branch

Sorry what branch do you mean?

@squidfunk

This comment has been minimized.

Copy link
Owner

squidfunk commented Mar 18, 2020

This one:
https://github.com/squidfunk/mkdocs-material/tree/refactor/rxjs-typescript

This is where v5 currently lives.

@jaimeiniesta jaimeiniesta changed the base branch from master to refactor/rxjs-typescript Mar 18, 2020
@jaimeiniesta

This comment has been minimized.

Copy link
Author

jaimeiniesta commented Mar 18, 2020

Alright, I need to fix those conflicts, I'll have it ready today.

@jaimeiniesta

This comment has been minimized.

Copy link
Author

jaimeiniesta commented Mar 18, 2020

I'm following https://deploy-preview-1486--mkdocs-material-preview.netlify.com/customization/#development-mode to run my development server but I can't get it to work - when I run npm start I see webpack compiling the assets but mkdocs serve doesn't seem to be started, as localhost:8000 is not accessible.

On the master branch, running npm run watch runs fine.

@squidfunk

This comment has been minimized.

Copy link
Owner

squidfunk commented Mar 18, 2020

You must start mkdocs serve in a separate terminal session. npm start will only start the Webpack watchdog. We should definitely update the docs on this topic.

EDIT: btw, MkDocs is not automatically started anymore because the dev server of MkDocs is..... IMHO rather bad. When there's a fresh Webpack build, it will queue up as many refreshes as there are new files, which is 5k since we added the icons to the repositories and then refresh the window every 5 seconds. Absolutely impractical. For this reason I've transitioned to starting the server after the initial build manually.

@Stanzilla

This comment has been minimized.

Copy link
Contributor

Stanzilla commented Mar 18, 2020

EDIT: btw, MkDocs is not automatically started anymore because the dev server of MkDocs is..... IMHO rather bad. When there's a fresh Webpack build, it will queue up as many refreshes as there are new files, which is 5k since we added the icons to the repositories and then refresh the window every 5 seconds. Absolutely impractical. For this reason I've transitioned to starting the server after the initial build manually.

Oh so that is what happens there. Yeah it's super annoying.

@jaimeiniesta

This comment has been minimized.

Copy link
Author

jaimeiniesta commented Mar 18, 2020

Thanks for the explanation. I can update the readme with these instructions on this PR as well.

@squidfunk

This comment has been minimized.

Copy link
Owner

squidfunk commented Mar 18, 2020

Sure, we should just add a paragraph to the customization guide saying that mkdocs serve must be started in a second session.

@Stanzilla

This comment has been minimized.

Copy link
Contributor

Stanzilla commented Mar 18, 2020

Can we not && chain it?

@jaimeiniesta

This comment has been minimized.

Copy link
Author

jaimeiniesta commented Mar 18, 2020

Can we not && chain it?

Don't think so, as the first command doesn't end until you stop it, so the second can't start.

@squidfunk

This comment has been minimized.

Copy link
Owner

squidfunk commented Mar 18, 2020

Exactly. I'm still working for a better approach to completely ditch the MkDocs development server, but didn't find something that works reasonably well yet.

@jaimeiniesta jaimeiniesta force-pushed the rocketvalidator:landmarks-unique branch from 4cd4129 to eed8d5c Mar 18, 2020
@netlify

This comment has been minimized.

Copy link

netlify bot commented Mar 18, 2020

Deploy preview for mkdocs-material-preview ready!

Built with commit eed8d5c

https://deploy-preview-1516--mkdocs-material-preview.netlify.com

@jaimeiniesta

This comment has been minimized.

Copy link
Author

jaimeiniesta commented Mar 18, 2020

OK I've fixed and force-pushed my branch, including the changes requested. Can you take another look please?

@jaimeiniesta

This comment has been minimized.

Copy link
Author

jaimeiniesta commented Mar 18, 2020

The netlify preview doesn't seem to be taking the latest changes in the branch, maybe because of the force-push?

@squidfunk

This comment has been minimized.

Copy link
Owner

squidfunk commented Mar 18, 2020

Looks good. Merging straight away. The only thing that I feel can be improved is the translation for "reset". I think we should call it "Clear". Is "Limpiar" the correct translation in Spanish?

@squidfunk

This comment has been minimized.

Copy link
Owner

squidfunk commented Mar 18, 2020

The netlify preview doesn't seem to be taking the latest changes in the branch, maybe because of the force-push?

This is because you did not build the theme. However, nevermind, I'll handle it.

@squidfunk squidfunk merged commit e7f7632 into squidfunk:refactor/rxjs-typescript Mar 18, 2020
2 checks passed
2 checks passed
build
Details
netlify/mkdocs-material-preview/deploy-preview Deploy preview ready!
Details
@jaimeiniesta

This comment has been minimized.

Copy link
Author

jaimeiniesta commented Mar 18, 2020

Looks good. Merging straight away. The only thing that I feel can be improved is the translation for "reset". I think we should call it "Clear". Is "Limpiar" the correct translation in Spanish?

Yes, Clear and Limpiar are OK.

Thanks! 🎉

@jaimeiniesta jaimeiniesta deleted the rocketvalidator:landmarks-unique branch Mar 18, 2020
@squidfunk

This comment has been minimized.

Copy link
Owner

squidfunk commented Mar 18, 2020

Funny side note: as my Spanish is very basic, I associate "limpiar" with the lady at the market asking me whether I want her to remove the guts of the squids I'm buying 😅

@squidfunk

This comment has been minimized.

Copy link
Owner

squidfunk commented Mar 18, 2020

The deploy preview was updated

@squidfunk

This comment has been minimized.

Copy link
Owner

squidfunk commented Mar 18, 2020

Oh and thanks for your help! Glad we could further improve accessibility

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.