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

Refine use of aria-label in Dashboard panel and top nav, Discover, and Management 'Edit index pattern' UI. #14341

Merged
merged 1 commit into from Oct 11, 2017

Conversation

cjcenizal
Copy link
Contributor

@cjcenizal cjcenizal commented Oct 6, 2017

This is a bit of a scattershot PR, but I just looked through our code for cases where we use aria-label and tried to apply spot fixes as I found issues.

@timroes
Copy link
Contributor

timroes commented Oct 6, 2017

I'm still a bit unsure about short vs. long aria-labels. Visually it totally makes sense to just print "Save" and "Load". They are places in the very top menu, and people (I assume - dunno if we have Usability tests to back that thesis) will understand, that this is a global action applying to the content of the complete page.

Since screen reader users have it a bit harder to get the overall context of an button they are currently in, and we also never have in any label for this menu, that all it's actions will apply to the dashboard and not maybe a single visualization they are viewing, or a dialog they might just have opened, I still think longer aria-labels could make sense here.

What are your opinions on that topic in general?

@cjcenizal
Copy link
Contributor Author

I agree with your thoughts @timroes. It's a tough judgment call depending on the situation. For example, the original audit suggested we change "Load saved search" to "Open", but I think that removes too much context (#11532 (comment)).

But for Dashboard, I feel like the context is pretty clear. The breadcrumbs communicate that you're looking at a dashboard (as opposed to a visualization). So my gut tells me that it's unlikely that they'll get confused the way you describe. But I don't feel strongly about this (2/10) so since it seems like you feel more strongly about it I'll change it back.

@cjcenizal cjcenizal force-pushed the improvement/aria-label branch 2 times, most recently from 67541fd to 94e31bb Compare October 6, 2017 17:19
Copy link
Contributor

@timroes timroes left a comment

Choose a reason for hiding this comment

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

Thanks. For these specific ones I would have also not too strong feelings (4/10 :D), but thought to bring up the topic in general. LGTM

@cjcenizal cjcenizal merged commit 9097a76 into elastic:master Oct 11, 2017
@cjcenizal cjcenizal deleted the improvement/aria-label branch October 11, 2017 17:41
cjcenizal added a commit to cjcenizal/kibana that referenced this pull request Oct 11, 2017
cjcenizal added a commit that referenced this pull request Oct 11, 2017
chrisronline pushed a commit to chrisronline/kibana that referenced this pull request Nov 20, 2017
chrisronline pushed a commit to chrisronline/kibana that referenced this pull request Dec 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants