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

"Reveal Password" functionality #84

Merged
merged 1 commit into from Feb 22, 2019

Conversation

@lmorchard
Copy link
Contributor

commented Feb 21, 2019

  • New PasswordText widget with hide/show buttons

  • Tweak hide/show and copy button alignments

  • Smaller PASSWORD_DOT

  • Replace hide / show eye icons

Fixes #20

Testing and Review Notes

  • Viewing an entry should now allow show / hide of password and not just simple dots
  • Show / hide button should no longer shift position on editing view

To Do

  • "long password" case does not grow wider in item view, exploring a solution

@lmorchard lmorchard requested a review from mozilla-lockwise/desktop-engineering as a code owner Feb 21, 2019

@ghost ghost assigned lmorchard Feb 21, 2019

@ghost ghost added the in progress label Feb 21, 2019

@lmorchard lmorchard force-pushed the lmorchard:20-reveal-password branch from 0074c9b to 85c5e33 Feb 21, 2019

@lmorchard
Copy link
Contributor Author

left a comment

Calling out a few different points of interest, mainly about my shaky CSS-fu.


import styles from "./input.css";

const PASSWORD_DOT = "\u2022";

This comment has been minimized.

Copy link
@lmorchard

lmorchard Feb 21, 2019

Author Contributor

This unicode char "•" more closely matches the Zeplin mock - but the previous value "●" more closely matches the built-in password field dots. This is easy to switch back if we want.

return (
<div {...props} className={classNames([
styles.monospace,
styles.passwordText,

This comment has been minimized.

Copy link
@lmorchard

lmorchard Feb 21, 2019

Author Contributor

Two issues I ran into here, trying to handle a "long password" case here:

  1. How long is long? (e.g. 15 characters?) We could possibly switch a classname on value length
  2. The whole form is on a shared CSS grid, with the copy buttons aligned. Breaking that alignment calls for a different overall layout for the form, I think? My CSS-fu fails me here.

This comment has been minimized.

Copy link
@linuxwolf

linuxwolf Feb 22, 2019

Member

@mozilla-lockbox/ux some insights here would be helpful

1. How long is long? (e.g. 15 characters?) We could possibly switch a classname on value length

I would start with 15 or 20 personally, too

2. The whole form is on a shared CSS grid, with the copy buttons aligned. Breaking that alignment calls for a different overall layout for the form, I think? My CSS-fu fails me here.

I think the intent of the designs is for the reveal button to be adjacent to the password text, which means it's position could shift based on how long the value is.

To keep the reveal in a fixed spot, I think you're right that it would require changing the CSS grid column count.

This comment has been minimized.

Copy link
@lmorchard

lmorchard Feb 22, 2019

Author Contributor

Well, I've got the reveal button sitting at the end of the text, which seems to match the spec. It's the unaligned copy button and expanding the width of the field that seems a problem

This comment has been minimized.

Copy link
@linuxwolf

linuxwolf Feb 22, 2019

Member

sorry, I was misunderstanding completely! And it looks like the UX questions are worked out.

background-repeat: no-repeat;
background-size: 16px 16px;
background-position: 0 2px;

This comment has been minimized.

Copy link
@lmorchard

lmorchard Feb 21, 2019

Author Contributor

This is kind of arbitrary, but seems to get the show/hide icon better aligned with the dots and midpoint of text. Might be worth yanking out.

}

.password-text {
width: 136px;

This comment has been minimized.

Copy link
@lmorchard

lmorchard Feb 21, 2019

Author Contributor

I copied a lot of these properties straight from Zeplin - some of it seems redundant and could probably get scrubbed out.

<path fill="#6D6D6E" d="M14.0160542,0.675736079 C14.3979633,1.05764518 14.3979633,1.67684218 14.0160542,2.05875128 L2.95193261,13.1228729 C2.5700235,13.504782 1.95082651,13.504782 1.56891741,13.1228729 C1.1870083,12.7409638 1.1870083,12.1217668 1.56891741,11.7398577 L12.633039,0.675736079 C13.0149481,0.293826977 13.6341451,0.293826977 14.0160542,0.675736079 Z"/>
<path fill="#6D6D6E" d="M9.60800171,4.46955872 C9.1423119,4.17216421 8.59352469,4 8,4 C6.34314575,4 5,5.34314575 5,7 C5,7.5968592 5.1629633,8.14651554 5.46343994,8.61383057 L9.60800171,4.46955872 Z"/>
</g>
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="16" height="16" viewBox="0 0 16 16">

This comment has been minimized.

Copy link
@lmorchard

lmorchard Feb 21, 2019

Author Contributor

Could probably run some optimization on these SVGs, but I just copied them out of the Zeplin mock

@lmorchard

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2019

Doh. Broke a handful of unit tests - handling that right now.

@lmorchard lmorchard force-pushed the lmorchard:20-reveal-password branch from 85c5e33 to fe3b64a Feb 21, 2019

@@ -65,19 +63,18 @@ export default class CopyToClipboardButton extends React.Component {
</Localized>
);
}
return (
<Stack stretch selectedIndex={selectedIndex} className={className}>

This comment has been minimized.

Copy link
@lmorchard

lmorchard Feb 21, 2019

Author Contributor

Oh yeah, and I meant to mention in my earlier comments that I found some of the CSS stuff a bit easier after dropping the Stack component (which was the main cause of the failed unit tests, so that reminded me)

"Reveal Password" functionality
- New PasswordText widget with hide/show buttons

- Tweak hide/show and copy button alignments

- Smaller PASSWORD_DOT

- Replace hide / show eye icons

Fixes #20

@lmorchard lmorchard force-pushed the lmorchard:20-reveal-password branch from fe3b64a to 6c9457b Feb 22, 2019

@nickbrandt

This comment has been minimized.

Copy link

commented Feb 22, 2019

After a discussion with @lmorchard regarding following our grid layout with the copy buttons, we have decided to move forward with the copy actions all staying aligned, even when one field is longer than the rest. The designs have been updated to reflect this.

@linuxwolf
Copy link
Member

left a comment

r+

Looks good to me.

@lmorchard lmorchard merged commit 35fe96f into mozilla-lockwise:master Feb 22, 2019

4 checks passed

WIP Ready for review
Details
codecov/patch 76.9% of diff hit (target 50%)
Details
codecov/project 95.6% (-0.4%) compared to 4d15207
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ghost ghost removed the in progress label Feb 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.