-
-
Notifications
You must be signed in to change notification settings - Fork 479
Improve keyboard and screen reader navigation on homepage #1667
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
Improve keyboard and screen reader navigation on homepage #1667
Conversation
43f3275
to
05f7306
Compare
Note that, even if this will have impact only after the bigger keyboard-related PR is merged, this can already be merged as is. It doesn't break anything. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/** | ||
* Helpers to create unstyled variants of various HTML elements that have default styles. | ||
* | ||
* Useful to easily build semantic content without having to deal with removing styles all the time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for this one. <3
So useful.
app/client/ui/HomeIntroCards.ts
Outdated
), | ||
}), | ||
// the now unused `webinarsLinks` is kept to prevent breaking existing translation strings | ||
unstyledH2(t('Learn more {{webinarsLinks}}', {webinarsLinks: ''})), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe not the best strategy but I chose to keep existing translations with an now obsolete param in it, rather than creating a new translation string. In order to prevent extra work on translations for no real value (except having clean, better self-explanatory code). What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer a clean, better self-explanatory code. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll change to have a new clean translation string and note for later that I'll change the translations myself to prevent extra work on people usually translating things.
edit: done
b109022
to
7d03167
Compare
fe4f432
to
8354cb3
Compare
8354cb3
to
ad4a67d
Compare
Deployed commit |
|
||
const title = `Version ${version.version}` + | ||
((version.gitcommit as string) !== 'unknown' ? ` (${version.gitcommit})` : ''); | ||
const altText = t('{{ organizationName }} - Back to home', { organizationName: this._appLogoOrg.get().name }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const altText = t('{{ organizationName }} - Back to home', { organizationName: this._appLogoOrg.get().name }); | |
const altText = t('{{ - organizationName }} - Back to home', { organizationName: this._appLogoOrg.get().name }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
woops, good catch
946ceac
to
ed6e959
Compare
Deployed commit |
- the grist version on hover has been moved to the grist icon in the footer. I'd say this rather hidden feature didn't have much value placed in the app header - now the app header home link correctly sets up an alt text for screen reader users, so that they know the link goes back to the current org homepage
Context
When full keyboard navigation will be possible, we'll want to target every link and button with keyboard only. Also, we'll want to understand everything if using a screen reader.
As for now, even when working on a grist version with the keyboard navigation PR code applied, some interactive elements here and there can't be targeted with keyboard. And some elements also don't have any text tied to them (some icon buttons, for example), making it pretty hard to understand things with a screen reader.
edit: the full keyboard navigation PR has been merged, so you can already see on the main branch the issues I talk about.
Proposed solution
This is a first pass to handle most cases I stumbled upon when on the grist homepage, listing our documents.
The whole idea is to:
Some things are not perfect (yet), but it's a good first pass. Every change should be pretty understandable, on its own commit.
note that, if you want to test the behavior right now, you should rebase this on my keyboard branch, as it is not merged yet and it's the code allowing normal browser tab navigation on the homepage.Has this been tested?
I didn't add any tests and wonder if I should right now ; those are lots of small technical changes. Maybe the best way to move forward would be to move along (updating current tests if they break). I'd like to, in parallel, try to include https://github.com/IBMa/equal-access in the tests suite to handle accessibility tests in a more global way.
Screenshots / Screencasts
Before this PR, (rebased on the keyboard PR branch):
keyboard-home-before.mp4
After this PR, (rebased on the keyboard PR branch):
keyboard-home-after.mp4