feat(text-input): add password visibility toggle for text fields #1060

emyarod · 2018-09-04T19:57:54Z

This PR will add classes and logic to support toggling password visibility for text fields. Adding this feature will lay the groundwork for extending the React component as well (https://github.com/IBM/carbon-components-react/issues/1033)

Changelog

New

password visibility toggle button
[data-text-input] attribute to find password text input form groups
.bx--text-input[data-toggle-password-visibility] selector to find the password input field
.bx--text-input--password__visibility selector to find the password visibility toggle button
.bx--text-input--password-visible selector for text fields with visible passwords
examples text fields with password visibility toggle in documentation

Changed

existing documentation examples styling

Testing / Reviewing

In a password field, enter some text in the text field and click the visibility toggle button to see changes. Examples can also be found in the documentation

<div class="bx--form-item" data-text-input>
  <label for="text-input-3" class="bx--label">Text Input label</label>
  <input
    id="text-input-3"
    type="password"
    class="bx--text-input"
    placeholder="Optional placeholder text"
    data-toggle-password-visibility
  >
  <button class="bx--text-input--password__visibility">
    <svg width="16" height="11" viewBox="0 0 16 11">
      <path d="M8 7.5c1.1 0 2-.9 2-2s-.9-2-2-2-2 .9-2 2 .9 2 2 2zm0 1c-1.7 0-3-1.3-3-3s1.3-3 3-3 3 1.3 3 3-1.3 3-3 3z"></path>
      <path d="M8 10c2.8 0 5.1-1.5 6.9-4.6C13.1 2.5 10.8 1 8 1 5.2 1 3 2.4 1.2 5.4 2.9 8.6 5.2 10 8 10zM8 0c3.3 0 6 1.8 8.1 5.4C14 9.2 11.3 11 8 11S2 9.2 0 5.5C2 1.9 4.6 0 8 0z"></path>
    </svg>
  </button>
</div>

alisonjoseph · 2018-09-06T18:33:14Z

@IBM/carbon-designers @IBM/carbon-developers

designertyler · 2018-09-06T20:48:30Z

Hi @emyarod A toggle should indicate the state rather than the action it will take. Similar to our toggle switches. When the password is hidden there is a slash though the eye. When the password is visible there should not be a slash.

emyarod · 2018-09-06T20:55:11Z

Thanks for the clarification @designertyler! I have updated my PR to reflect this

asudoh

Thank you for bringing up an idea and great to see almost 💯 JS code @emyarod!

asudoh · 2018-09-06T23:10:20Z

src/components/text-input/text-input.js

+  _toggle({ element, button }) {
+    /* eslint-disable max-len */
+    const iconVisibilityOff =
+      '<svg width="16" height="11" viewBox="0 0 16 11"><path d="M8 7.5c1.1 0 2-.9 2-2s-.9-2-2-2-2 .9-2 2 .9 2 2 2zm0 1c-1.7 0-3-1.3-3-3s1.3-3 3-3 3 1.3 3 3-1.3 3-3 3z"></path><path d="M8 10c2.8 0 5.1-1.5 6.9-4.6C13.1 2.5 10.8 1 8 1 5.2 1 3 2.4 1.2 5.4 2.9 8.6 5.2 10 8 10zM8 0c3.3 0 6 1.8 8.1 5.4C14 9.2 11.3 11 8 11S2 9.2 0 5.5C2 1.9 4.6 0 8 0z"></path></svg>';


Can we toggle hidden attribute for those icons instead of changing HTML here?

@asudoh I've added another class method setIconVisibility to handle changing the display attribute rather than changing the markup. Let me know what you think

Good try @emyarod! .style.x causes usage of inline style (something we avoid) though - So we need to seek for an alternate approach like hidden attribute.

asudoh · 2018-09-06T23:11:44Z

src/components/text-input/text-input.js

+   * The component options.
+   *
+   * If `options` is specified in the constructor,
+   * {@linkcode NumberInput.create .create()},


Let's search NumberInput refs in this file and replace them with TextInput.

asudoh · 2018-09-06T23:13:51Z

src/components/text-input/text-input.hbs

 </div>

-<div class="bx--form-item">
+<div data-text-input class="bx--form-item">


Nitpick: Do we want to prevent TextInput JS class from being instantiated for non-password text inputs? 🤔

I added the [data-text-input] on div.bx--form-item in order to differentiate text inputs from other components that are also wrapped with div.bx--form-item. Some of those components like Checkbox and ComboBox also have identifying classes on that div like .bx--combo-box or .bx--checkbox-wrapper, but other components like DatePicker or TextInput do not.

My thinking was that it might be convenient for users that want to select the TextInput div.bx--form-item, but in retrospect it might be better to use something like .bx--text-input-wrapper rather than a data attribute. And if we do add a wrapper class, then I think we should prevent TextInput JS class from being instantiated for non-password text inputs. So a password text input would be div.bx--form-item[data-text-input] while a non-password text input would be div.bx--form-item.bx--text-input-wrapper. What do you think @asudoh?

@emyarod Thank you for putting your thoughts on this! I think it makes sense.

claycrenshaw · 2018-09-07T19:46:19Z

@emyarod @designertyler I'm not so sure about this call on the show/hide password icon. This article touches on the topic: https://ux.stackexchange.com/questions/54162/icon-for-unmasking-passwords-open-or-closed-to-begin-with. I agree with the consensus there that since this is an action (show or hide password), that the icon should represent the result of the action, and not the current state.

designertyler · 2018-09-07T20:13:46Z

@claycrenshaw In our case the show/hide toggle is an option to change the state of the password field and the action is to log in or cancel. In that example shuffle/linear is an option to change the state of playback and play or pause is the action to stop or start music.

In both cases the option (eye/shuffle) should show the active state and the action (play/log in) should show what action will happen.

edit I think the stack overflow logic is correct, but they mistakingly call the eye toggle an action.

emyarod · 2018-09-07T20:48:15Z

so just to be clear: the password visibility button behavior is analogous to a checkbox, right? (in that the current state of the component is shown, as opposed to the result of clicking on the component being shown)

I've currently implemented it according to @designertyler's previous description, but this is an interesting topic to discuss. I will note, though, that the pattern described can also be seen in Material Design 2.0 login forms

chrisconnors-ibm · 2018-09-07T21:08:48Z

It's not a label; the state hidden or visible is self evident by the value displayed, either the password or •••••••••••••

it's a command - two commands, actually:

slash eye = hide
open eye = show

so the hide command only makes sense when the password value is visible;
the show command only makes sense when the password value is hidden.

And yes, Material has this backwards. Let's not perpetuate their mistakes.

emyarod · 2018-09-07T21:57:15Z

thank you for the feedback @chrisconnors-ibm! Alright in that case then, my original implementation as shown in the GIF below would be the direction we want to proceed with, is that correct?

asudoh

Getting there in code wise - Great to have a movie of the demo here so our designers can give this a final green light - Thanks @emyarod!

asudoh · 2018-09-10T19:56:30Z

src/components/text-input/_text-input.scss

@@ -74,6 +73,35 @@
        display: none;
      }
    }
+
+    &-wrapper svg[hidden='true'] {


The existence hidden attribute should be treated as hidden - So can we do: svg[hidden]?

sure, I have refactored it so that it will remove the hidden attribute entirely when we want to show the icon

I was initially selecting svg[hidden="true"] here because I was toggling between hidden="true" and hidden="false", but now I can select svg[hidden] since the attribute will either be set or removed

asudoh · 2018-09-10T19:57:47Z

src/components/text-input/text-input.js

+    // toggle action must come before querying the classList
+    element.classList.toggle(this.options.passwordIsVisible);
+    const passwordIsVisible = element.classList.contains(this.options.passwordIsVisible);
+    const iconVisibilityOn = button.querySelector('svg.icon--visibility-on');


Let’s put those selector in options list.

asudoh

Thanks @emyarod for your quick changes! LGTM 👍 from code wise. Please make sure sharing your work with our designers (e.g. by putting a movie in this PR or running gulp build:dev --rollup -e; cf push in your project directory) so they can sign this off.

emyarod · 2018-09-11T20:05:33Z

@asudoh I'm not familiar with deploying to cloudfoundry, but I do have a GIF of the latest component behavior here, if that works:

Are there any instructions or guides for pushing to cf? Also, is there anything I should modify with regards to the two remaining topics at the end of the PR description?

asudoh · 2018-09-12T06:48:14Z

@emyarod If you are familiar with CF, here's everything you need:

gulp build:dev --rollup -e
cf login ...
cf push

Ping me if it doesn't work for you and I can help you out!

emyarod · 2018-09-12T20:41:40Z

thanks for your help @asudoh! this PR can be viewed at this link: https://carbon-dev-environment-relaxed-roan.mybluemix.net/demo/text-input

fix(text-input): revert behavior of visibility icon on click

asudoh

Signing off, thanks @emyarod for your hard work on this and everybody for reviewing! Anybody don't hesitate to speak up in case I missed any pending discussions - Thanks!

carbon-bot · 2018-09-20T14:08:50Z

🎉 This PR is included in version 9.17.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

…n-system#1060) * feat(Checkbox): add `title` attribute to checkbox label * fix(Checkbox): avoid empty title attribute

emyarod force-pushed the toggle-password-visibility branch from 83a1a0d to cb3fae9 Compare September 4, 2018 19:58

alisonjoseph added the status: WIP 👩‍💻 label Sep 5, 2018

emyarod force-pushed the toggle-password-visibility branch 3 times, most recently from 78cca1a to b179020 Compare September 6, 2018 16:42

emyarod changed the title ~~[WIP] feat(text-input): add password visibility toggle for text fields~~ feat(text-input): add password visibility toggle for text fields Sep 6, 2018

emyarod changed the title ~~feat(text-input): add password visibility toggle for text fields~~ [WIP] feat(text-input): add password visibility toggle for text fields Sep 6, 2018

emyarod force-pushed the toggle-password-visibility branch from 9d2fe7a to 2fa48ab Compare September 6, 2018 18:46

asudoh reviewed Sep 6, 2018

View reviewed changes

emyarod force-pushed the toggle-password-visibility branch 2 times, most recently from ee1db7f to 6a594bc Compare September 7, 2018 19:35

emyarod force-pushed the toggle-password-visibility branch from 6a594bc to 811f69c Compare September 7, 2018 20:58

asudoh reviewed Sep 10, 2018

View reviewed changes

emyarod force-pushed the toggle-password-visibility branch from 0974fda to de3c3fc Compare September 11, 2018 13:56

asudoh reviewed Sep 11, 2018

View reviewed changes

emyarod force-pushed the toggle-password-visibility branch from de3c3fc to e927794 Compare September 11, 2018 19:53

alisonjoseph added the status: visual review 👀 label Sep 11, 2018

emyarod force-pushed the toggle-password-visibility branch 2 times, most recently from dfd3c33 to d43d04f Compare September 12, 2018 20:24

alisonjoseph added status: one more review 👀 and removed status: WIP 👩‍💻 status: visual review 🔍 labels Sep 19, 2018

emyarod added 15 commits September 19, 2018 15:31

feat(text-input): add password visibility toggle

5a8dbe2

docs(text-input): add README for text input

a5ad0a9

feat(text-input): add primary and hover colors to toggle SVG

b40687b

test(text-input): add unit test for text-input

392b88a

test(text-input): select data-text-input rather than bx--form-item

22674ba

chore(.all-contributorsrc): add @emyarod as a contributor

6810856

fix(text-input): indicate current state of password visibility

13a1243

docs(text-input): replace NumberInput refs with TextInput

8d02f6a

refactor(text-input): toggle icon display value over changing HTML

5a8fbc7

fix(text-input): eliminate gap between text field and validation message

9579789

style(text-input): reorganize conditional logic for password/text fields

a9f3c64

refactor(text-input): toggle hidden attribute instead of changing CSS

5e0b8e6

fix(text-input): revert behavior of visibility icon on click

test(text-input): only test on password visibility markup

2e59186

refactor(text-input): remove hidden attr instead of setting to false

f30c4f5

refactor(text-input): move svg icon classes to options object

8ea8579

emyarod force-pushed the toggle-password-visibility branch from 1ae2d57 to 8ea8579 Compare September 19, 2018 20:31

asudoh approved these changes Sep 20, 2018

View reviewed changes

alisonjoseph merged commit 57257f2 into carbon-design-system:master Sep 20, 2018

This was referenced Sep 20, 2018

feat(TextInput): add password visibility toggle for text fields carbon-design-system/carbon-components-react#1344

Closed

docs(text-input): add light themed password input example #1145

Merged

emyarod deleted the toggle-password-visibility branch November 16, 2018 22:19

This was referenced Feb 6, 2019

Experimental password inputs broken #1734

Closed

fix(text-input): position experimental password visibility toggle #1735

Merged

joshblack pushed a commit to joshblack/carbon that referenced this pull request May 2, 2019

feat(Checkbox): add title attribute to checkbox label (carbon-desig…

7bee608

…n-system#1060) * feat(Checkbox): add `title` attribute to checkbox label * fix(Checkbox): avoid empty title attribute

emyarod mentioned this pull request May 6, 2019

feat(TextInput): add password visibility toggle for text fields carbon-design-system/carbon-components-react#2323

Merged

emyarod mentioned this pull request Jul 25, 2019

Recommend Adding a Password Field to Carbon #3512

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(text-input): add password visibility toggle for text fields #1060

feat(text-input): add password visibility toggle for text fields #1060

emyarod commented Sep 4, 2018 •

edited

Loading

alisonjoseph commented Sep 6, 2018

designertyler commented Sep 6, 2018

emyarod commented Sep 6, 2018

asudoh left a comment

asudoh Sep 6, 2018

emyarod Sep 7, 2018

asudoh Sep 7, 2018

asudoh Sep 6, 2018

asudoh Sep 6, 2018

emyarod Sep 7, 2018

asudoh Sep 7, 2018

claycrenshaw commented Sep 7, 2018

designertyler commented Sep 7, 2018 •

edited

Loading

emyarod commented Sep 7, 2018 •

edited

Loading

chrisconnors-ibm commented Sep 7, 2018

emyarod commented Sep 7, 2018

asudoh left a comment

asudoh Sep 10, 2018

emyarod Sep 11, 2018

asudoh Sep 10, 2018

asudoh left a comment

emyarod commented Sep 11, 2018 •

edited

Loading

asudoh commented Sep 12, 2018

emyarod commented Sep 12, 2018

asudoh left a comment

carbon-bot commented Sep 20, 2018

feat(text-input): add password visibility toggle for text fields #1060

feat(text-input): add password visibility toggle for text fields #1060

Conversation

emyarod commented Sep 4, 2018 • edited Loading

Changelog

Testing / Reviewing

alisonjoseph commented Sep 6, 2018

designertyler commented Sep 6, 2018

emyarod commented Sep 6, 2018

asudoh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

claycrenshaw commented Sep 7, 2018

designertyler commented Sep 7, 2018 • edited Loading

emyarod commented Sep 7, 2018 • edited Loading

chrisconnors-ibm commented Sep 7, 2018

emyarod commented Sep 7, 2018

asudoh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asudoh left a comment

Choose a reason for hiding this comment

emyarod commented Sep 11, 2018 • edited Loading

asudoh commented Sep 12, 2018

emyarod commented Sep 12, 2018

asudoh left a comment

Choose a reason for hiding this comment

carbon-bot commented Sep 20, 2018

emyarod commented Sep 4, 2018 •

edited

Loading

designertyler commented Sep 7, 2018 •

edited

Loading

emyarod commented Sep 7, 2018 •

edited

Loading

emyarod commented Sep 11, 2018 •

edited

Loading