Skip to content
This repository has been archived by the owner on Oct 8, 2024. It is now read-only.

Add title tag #41

Merged
merged 6 commits into from
Jan 7, 2016
Merged

Add title tag #41

merged 6 commits into from
Jan 7, 2016

Conversation

jeremykenedy
Copy link
Contributor

Added title tag to toggle attr, show, and hidden states.

@tylersticka
Copy link
Member

@jeremykenedy Thanks for contributing this PR!

It would be helpful to have a little more context for these changes. Currently, the button has an aria-label of "Show Password" and the aria-pressed attribute is toggled to true or false. My concern with these changes is that if the title is ever set to Hide Password while aria-pressed is true, that it will seem as if clicking the button will do the opposite of what it does.

But if this change was fueled by actual usability issues you encountered while using accessibility technologies, we definitely want to hear about it!

@jeremykenedy
Copy link
Contributor Author

I introduced the title tag due to when only using the icon for toggle there is no indicator as to what it does other than the icon, with the title tag it will give you the "hide" and "show" verbiage on hover.

Also, the JS will toggle the title text when clicked.

@@ -112,6 +112,7 @@
attr: {
role: 'button',
'aria-label': 'Show Password',
'title': 'Show Password',
Copy link
Member

Choose a reason for hiding this comment

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

Unquote 'title' (legal prop name does not require quotes).

@tylersticka
Copy link
Member

@jeremykenedy Thanks for responding to my question!

I had to really think this over. Initially I was skeptical because an icon-only treatment is an implementation choice on the part of the developer, and the code already allows the implementor to pass along custom attributes for each state (though the syntax for doing so is pretty verbose).

That said, the alternative text for this is simply "Show" and "Hide" within the element itself, which may not be enough to clue user's in to what is actually going on. So as of this morning I'm coming around to this being a nice default to implement.

I've pointed out a few small formatting nits inline. If you wouldn't mind making those adjustments, I'd be happy to merge this in and give it a version bump.

@jeremykenedy
Copy link
Contributor Author

Hello, thanks for letting me contribute :) I have made the changes. Cheers!

tylersticka added a commit that referenced this pull request Jan 7, 2016
@tylersticka tylersticka merged commit 5cbc68a into cloudfour:master Jan 7, 2016
@tylersticka
Copy link
Member

Thanks, @jeremykenedy! Your changes are in version 2.0.9.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants