-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
InputControl: added password visibility story #60898
InputControl: added password visibility story #60898
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
<Tooltip | ||
text={ visible ? 'Hide password' : 'Show password' } | ||
> | ||
<Button |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be better to use the size="small"
variant so the focus rings don't overlap.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what's the right way to style things in this context, so I went with inline styles to vertically center it. I also originally went with flex + align items center, but then realized flex was enough (not sure why though). Any suggestions welcome!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's just the fact the Button became display: block
because of the flex parent. Which is equivalent to setting line-height: 0
.
An inline style is perfectly appropriate here 👍
Just FYI because this happens a lot: After there is a new release of the wp-components package on trunk, any open PRs with changelog modifications that haven't been manually reconciled with trunk can get placed under the wrong release section upon landing, due limitations of the automatic text merging. 7e0f0ea Let me know if you have any ideas how to prevent that from happening 😅 For now we have to keep an eye out for releases (about twice monthly), and reconcile manually. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thank you!
What?
Fixes #60502
Add a story with an example of a "password input visibility toggle" pattern, as suggested in linked issue.
Why?
To provide users with an example of how to implement this pattern.
How?
Adding a story.
Testing Instructions
See InputControl > Show Password story on Storybook.
Testing Instructions for Keyboard
Tab around to focus the input or the button.
Screenshots or screencast
Kapture.2024-04-19.at.14.19.09.mp4