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.

Sign up for GitHub

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

Jump to bottom

[EuiFieldPassword] Support toggling of obfuscation #3751

Merged
merged 14 commits into from
Jul 27, 2020
Merged

[EuiFieldPassword] Support toggling of obfuscation #3751

merged 14 commits into from
Jul 27, 2020

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Jul 15, 2020 •
edited
Loading

Fixes #3153

Based on guidance mentioned in the issue and this reference, the easiest way to support is with a button icon that changes the input type from password to text.

Therefore, this PR simply adds a new prop called type: 'password' | 'text' | 'dual'. The default being 'password.'

The 'text' option is simply there to allow consumers to handle the text masking manually from outside of the component. The 'dual' option adds an EuiButtonIcon with eye/eyeClosed icons to the append of the form layout which simply toggles the type from 'password' to 'text'.

Screen Recording 2020-07-15 at 04 42 28 PM

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in IE11 and Firefox
  • Props have proper autodocs
  • Added documentation
  • Checked Code Sandbox works for the any docs examples
  • Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3751/

@cchaos
Copy link
Contributor Author

cchaos commented Jul 16, 2020

Currently just waiting on #3749

@cchaos cchaos force-pushed the prop/show_password branch from 8baa109 to 5e64ef3 Compare July 21, 2020 15:25
Quick non-related fix, remove zindex completely from popovers in da…
8ce56ce 
…tagrid example

Unnecessary to have a custom z-index as the default is much higher than the supplied one.
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3751/

@cchaos
Copy link
Contributor Author

cchaos commented Jul 21, 2020

Dependency merged, opening this up for review.

@cchaos cchaos marked this pull request as ready for review July 21, 2020 15:52
@cchaos
Copy link
Contributor Author

cchaos commented Jul 21, 2020

@myasonik Adding you as a reviewer, but always as optional.

myasonik
myasonik approved these changes Jul 21, 2020
View reviewed changes
Copy link
Contributor

@myasonik myasonik left a comment

Choose a reason for hiding this comment

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

Looked good to me with a quick pass just with keyboard and VO+ChromiEdge. Didn't go deep on it though.

Will ask that someone proof this in Windows though! I'm like 80% certain that Windows adds its own reveal button to password fields so I'd be curious to see how these interact.

@cchaos
Copy link
Contributor Author

cchaos commented Jul 21, 2020

Will ask that someone proof this in Windows though! I'm like 80% certain that Windows adds its own reveal button to password fields so I'd be curious to see how these interact.

Hrrmph. I'll do a little research.

@cchaos
Copy link
Contributor Author

cchaos commented Jul 21, 2020

The only thing I could find was this. https://developer.mozilla.org/en-US/docs/Archive/Web/CSS/::-ms-reveal And that box makes me side-eye implementing it...

@myasonik
Copy link
Contributor

🤦 I'm having a foggy-brain day... I thought it existed because I saw it moments ago on ChromiEdge. And I just verified it the same on my Windows machine.

Screenshot of Edge's extra password reveal button

Can we hide the browser one with ::ms-reveal { display: none; }? Yours seems to be an improvement on it anyways because I couldn't find a way to navigate to it by keyboard and could only toggle it with my mouse.

@cchaos
Copy link
Contributor Author

cchaos commented Jul 21, 2020

I could, just a bit wary of actually using it based on the link I mentioned above and the big ole warning box wrapped around the whole thing

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3751/

@thompsongl
Copy link
Contributor

thompsongl commented Jul 21, 2020 •
edited
Loading

That warning box is something else. I use MDN docs all the time and have never seen that style 😄

Do not use it on production sites facing the Web: it will not work for every user.

This, however, makes me think there may not be any severe effects for our case as we're not relying on it for anything beyond a visual experience. Worst case is that a user sees both. Not great, but it doesn't seem dangerous. Thoughts?

@cchaos
Copy link
Contributor Author

cchaos commented Jul 22, 2020

Yeah I think so long as we're not relying on it for every user, and I'll only remove it if we're providing the custom one.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3751/

thompsongl
thompsongl reviewed Jul 22, 2020
View reviewed changes
Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

I formalized the multiple ref thing you had. There's another open PR with the same pattern, so I made a hook for it: cchaos#35

elizabetdev
elizabetdev approved these changes Jul 23, 2020
View reviewed changes
Copy link
Contributor

@elizabetdev elizabetdev left a comment

Choose a reason for hiding this comment

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

Tested in FF, Chrome, and Safari and LGTM! 🎉

I just noticed that in Safari the placeholder is not vertically centered. But this is happening in all input fields:

Screenshot 2020-07-23 at 17 03 54

@cchaos
Copy link
Contributor Author

cchaos commented Jul 23, 2020

I just noticed that in Safari the placeholder is not vertically centered. But this is happening in all input fields:

Can you make an issue?

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3751/

@thompsongl
Copy link
Contributor

I think if you use the /* */ format it will show up in editor hints, otherwise I don't think this format does.

That's good to know. Thanks for fixing locally

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3751/

@cchaos cchaos merged commit fa9e626 into elastic:master Jul 27, 2020
@thompsongl thompsongl mentioned this pull request Jul 31, 2020
Merged
@mmourafiq mmourafiq mentioned this pull request Aug 10, 2020
Closed
@cchaos cchaos deleted the prop/show_password branch September 11, 2020 15:39
@legrego legrego mentioned this pull request Sep 14, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thompsongl thompsongl thompsongl approved these changes

@elizabetdev elizabetdev elizabetdev approved these changes

@myasonik myasonik myasonik approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

EuiFieldPassword missing the prop 'isVisible'.
5 participants
@cchaos @kibanamachine @myasonik @thompsongl @elizabetdev