feat(password-input): allow toggling password visibility when readonly is true #21158
Conversation
maradwan26
changed the title
maradwan26
added
package: react
maradwan26
requested review from
a team and
sunny-babbar
and removed request for
a team
✅ Deploy Preview for v11-carbon-web-components ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #21158 +/- ##
==========================================
- Coverage 85.17% 85.10% -0.08%
==========================================
Files 530 530
Lines 40889 40903 +14
Branches 6280 6277 -3
==========================================
- Hits 34829 34809 -20
- Misses 5900 5934 +34
Partials 160 160
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for v11-carbon-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
@maradwan26 Thanks for making this PR. It is looking good but there are just a couple things that need to be changed to match the design spec.
I feel eventually design should just use the icon button styling for the view icon here, but this is a design change we need to look at at a larger scale across the system for these types of situations, so we will look at that at a later date.
React
Default style
- In readOnly, The view icon should not get a visual color change on hover. It should always be
$icon-primary, on enabled and hover states. - The tooltip should also be using the icon button tooltip height. The alignment of the caret on the tooltip is also off and it should match how WC is doing it. The spacing between the button and the tooltip should be 4px. I have a feeling this is a problem with the icon button component in React. That can be addressed in a separate PR if it needs to be.
Fluid style
- In readOnly, The input text and dots should be
$text-secondaryfor the fluid style. - In readOnly, The view icon should not get a visual color change on hover. It should always be
$icon-primary. This also should be the same in the enabled state. - In the tooltip, I would just say “Hide password” and “Show password” like we do for the Default style for consistency.
- Same tooltip issue as above in the default style, but I think its a problem in the icon button component in React.
WC
Default style
- In readOnly, The view icon should not get a visual color change on hover. It should always be
$icon-primary, on enabled and hover states. - In the tooltip, Keep the
pin "password" lowercase to match React and our content guidelines. - The tooltip should also be using the icon button tooltip height. That can be addressed in a separate PR if it needs to be.
Like you mentioned earlier, we can review Fluid in WC when it is complete.
|
Applied changes! 🚀
Done ✅
Tooltip height and spacing is fixed. 
I think alignment is fixed too since it looks the same for React and WC.
Done ✅ , fixed it for fluid textinput as well
Done ✅
Fixed ✅ , changes from the default passwordInput were carried over
Done ✅ |
There was a problem hiding this comment.
@maradwan26 All of the changes you made look great, thank you!
The only thing left I see is that in WC, the tooltip does not need the dropshadow behind it. Other than that everything looks good to go.
|
@laurenmrice dropShadow removed! 🚀 |
kennylam
left a comment
kennylam
left a comment
There was a problem hiding this comment.
Looks good, just a little optimization needed.
| firstUpdated() { | ||
| this.updateComplete.then(() => { | ||
| this.shadowRoot | ||
| ?.querySelector(`${prefix}-tooltip`) | ||
| ?.shadowRoot?.querySelector(`.${prefix}--tooltip`) | ||
| ?.classList.add(`${prefix}--icon-tooltip`); | ||
| }); | ||
| } |
There was a problem hiding this comment.
// will need to import `query` from lit/decorators.js
@query(`${prefix}-tooltip`)
private _passwordTooltip?: HTMLElement;
...
firstUpdated() {
this._passwordTooltip
?.shadowRoot
?.querySelector(`.${prefix}--tooltip`)
?.classList.add(`${prefix}--icon-tooltip`);
}The updateComplete here isn't needed, since firstUpdated runs after the initial render. Also I think that deep query can be cleaned up a bit and made more efficient with a cached query of prefix-tooltip.
7 participants
Closes #21116
Updates the PasswordInput
readOnlystate to allow for the user to toggle the password visibility in both React and Web ComponentsChangelog
New
Changed
disabledis true and excludereadOnlyTesting / Reviewing
readOnlyis trueNote
The implementation for the WC FluidPasswordInput is still in progress, however, similarly to the React version, the
readOnlystate handling is covered by the base PasswordInput. I pulled down that branch and tested the changes there, and it functionally works, so I believe once that PR is merged it should be goodPR Checklist
As the author of this PR, before marking ready for review, confirm you:
[ ] Updated documentation and storybook examples