- Notifications
You must be signed in to change notification settings - Fork5.5k
Toolbar buttons tooltip: show help instead of label#5107
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
lresende commentedJan 21, 2020
What is the accessibility side effect of this change?@jasongrout@takluyver any thoughts here? |
kevin-bates commentedMay 5, 2020
@jasongrout - can you please comment on Luciano's accessibility question? Otherwise, I think this looks good. |
kevin-bates left a comment
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.
My review is based purely on the excellent screenshots - thank you.
There's still the question of accessibility that should be resolved.
kevin-bates commentedMay 5, 2020
@tgeorgeux - I believe you deal in the realm of UX. Do these changes introduce any accessibility issues and, if so, might there be a way they can be addressed while still providing this improved behavior? |
0d592d3 tod7f86efCompare
afshin left a comment
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 for the PR!
I rebased your branch againstmaster and pushed. I would suggest adding anaria-label and also defaulting to theel.label in cases where help is not available and just switching the default order instead of removing it altogether. Something like this diff:
diff --git a/notebook/static/notebook/js/toolbar.js b/notebook/static/notebook/js/toolbar.jsindex 0e47e3470..295975c30 100644--- a/notebook/static/notebook/js/toolbar.js+++ b/notebook/static/notebook/js/toolbar.js@@ -94,7 +94,8 @@ define(['jquery','base/js/i18n'], function($, i18n) { } var button = $('<button/>') .addClass('btn btn-default')- .attr("title", i18n.msg._(action.help))+ .attr("aria-label", el.label)+ .attr("title", i18n.msg._(action.help)||el.label) .append( $("<i/>").addClass(el.icon||(action||{icon:'fa-exclamation-triangle'}).icon).addClass('fa') );I'm happy to push a commit with this myself since this PR was opened quite some time ago and you may be otherwise occupied.
tgeorgeux commentedMay 17, 2020
@kevin-bates Sorry I missed this ping. I don't really know enough about the mechanics of what's going on to know if this change will negatively impact accessibility. It looks like a net improvement to me, with@afshin's changes mentioned above. |
38689d8 to5d47947Comparemanics commentedMay 17, 2020
Thanks for looking, I've made the requested change. |
afshin commentedMay 17, 2020
Thank you! |
Currently the tooltip for a toolbar button with just an icon is the help text for the action/command.
If a toolbar button has a label the label text is shown alongside the button icon as expected, but it also replaces the tooltip help.
With this PR the help text is always shown as the tooltip.
Old behaviour:
New behaviour: