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

feat(fields/password): add show/hide button #555

Merged
merged 2 commits into from
Feb 20, 2019
Merged

feat(fields/password): add show/hide button #555

merged 2 commits into from
Feb 20, 2019

Conversation

jonnybel
Copy link
Contributor

@jonnybel jonnybel commented Feb 19, 2019 •
edited
Loading

Summary

  • Added new Eye icon
  • Added show/hide button to PasswordField

Closes #551.

Description

Behaviour:

kapture 2019-02-19 at 11 43 55

Removed the isPasswordVisible prop from the PasswordField.

emmenko
emmenko requested changes Feb 19, 2019
View reviewed changes
Copy link
Member

@emmenko emmenko left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. The approach is good, you just forgot to do a couple of things 😉

montezume
montezume reviewed Feb 19, 2019
View reviewed changes
src/components/icons/index.js
@@ -56,6 +56,7 @@ import OrigExpandIcon from './svg/expand.react.svg';
import OrigExportIcon from './svg/export.react.svg';
import OrigExternalLinkIcon from './svg/external-link.react.svg';
import OrigEyeCrossedIcon from './svg/eye-crossed.react.svg';
import OrigEyeIcon from './svg/eye.react.svg';
Copy link
Contributor

Choose a reason for hiding this comment

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

need to remember to document this for the changelog

tdeekens
tdeekens previously approved these changes Feb 19, 2019
View reviewed changes
Copy link
Contributor

@tdeekens tdeekens left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Code looks good to me. Thanks. Only blocked by the other PR and the state.

@tdeekens tdeekens dismissed their stale review February 19, 2019 15:00

I think we are missing the initial state still.

emmenko
emmenko reviewed Feb 19, 2019
View reviewed changes
@montezume montezume added the 🚀 Type: New Feature Something new label Feb 19, 2019
emmenko
emmenko reviewed Feb 19, 2019
View reviewed changes
Copy link
Member

@emmenko emmenko left a comment

Choose a reason for hiding this comment

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

Can you also add a VRT for showing the password?

@tdeekens
Copy link
Contributor

Can you also add a VRT for showing the password?

I think we agreed with Malcolm to have it on the input not group. The group can test it in a rtl spec.

@montezume
Copy link
Contributor

@emmenko I don't think it makes sense to an interaction based test for showing password (ie, click with puppeteer, and then take percy snapshot), as all we would be testing is that the text changes, and the prop is passed to PasswordInput.

We already visually test Password input in those states based on the isPasswordVisible prop.

@jonnybel jonnybel force-pushed the ja-show-password-btn branch from 953f4d7 to 4f3626f Compare February 20, 2019 08:50
emmenko
emmenko approved these changes Feb 20, 2019
View reviewed changes
Copy link
Member

@emmenko emmenko left a comment

Choose a reason for hiding this comment

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

Alright, it's good for me then!

montezume
montezume approved these changes Feb 20, 2019
View reviewed changes
Copy link
Contributor

@montezume montezume left a comment

Choose a reason for hiding this comment

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

Everything looks good to me! Maybe just need a design review of the percy diffs? 🙏

@emmenko
Copy link
Member

emmenko commented Feb 20, 2019

Is someone from design team reviewing the VRT diffs?

qmateub
qmateub approved these changes Feb 20, 2019
View reviewed changes
Copy link
Contributor

@qmateub qmateub left a comment

Choose a reason for hiding this comment

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

Nice 🎢 !

@tdeekens
Copy link
Contributor

/cc @filippobocik can you check the VRT diffs?

@filippobocik
Copy link

Works great, thanks! One little addition: If the PasswordField is disabled or read only, the show / hide button should be hidden. Gonna check out Percy afterwards.

@jonnybel jonnybel force-pushed the ja-show-password-btn branch from 4f3626f to 85539aa Compare February 20, 2019 09:41
@jonnybel
Copy link
Contributor Author

@filippobocik I've updated the behaviour.

@montezume montezume merged commit 9f420c5 into master Feb 20, 2019
@montezume montezume deleted the ja-show-password-btn branch February 20, 2019 10:16
@tdeekens
Copy link
Contributor

Good stuff @jonnybel! ⛱ 🎉

@montezume
Copy link
Contributor

Keeping your commits to match the changelog entries was 🔥 💯 @jonnybel

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emmenko emmenko emmenko approved these changes

@tdeekens tdeekens tdeekens approved these changes

@montezume montezume montezume approved these changes

@qmateub qmateub qmateub approved these changes

Assignees
No one assigned
Labels
🚀 Type: New Feature Something new
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add "Show/Hide" toggle button to Password input
6 participants
@jonnybel @tdeekens @montezume @emmenko @filippobocik @qmateub