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(PasswordInput): sync input type prop and state #8368

Merged
merged 6 commits into from
May 13, 2021
Merged

feat(PasswordInput): sync input type prop and state #8368

merged 6 commits into from
May 13, 2021

Conversation

emyarod
Copy link
Member

@emyarod emyarod commented Apr 12, 2021

This PR deprecates TextInput.ControlledPasswordInput and adds support for control over the password input field type in the TextInput.PasswordInput

this PR includes changes from #8366 so we should review and merge those changes first

Testing / Reviewing

Confirm that the controlled password input story which uses TextInput.PasswordInput instead of TextInput.ControlledPasswordInput functions as expected

@emyarod emyarod force-pushed the deprecate-controlled-password-input branch 2 times, most recently from c204da4 to 9fce269 Compare April 12, 2021 18:05
@netlify
Copy link

netlify bot commented Apr 12, 2021 •
edited
Loading

Deploy preview for carbon-elements ready!

Built with commit f1e490b

https://deploy-preview-8368--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Apr 12, 2021

Deploy preview for carbon-components-react ready!

Built with commit c204da4

https://deploy-preview-8368--carbon-components-react.netlify.app

@netlify
Copy link

netlify bot commented Apr 12, 2021 •
edited
Loading

Deploy preview for carbon-components-react failed.

Built without sensitive environment variables with commit f1e490b

https://app.netlify.com/sites/carbon-components-react/deploys/609d44d48464690007775a27

@emyarod emyarod mentioned this pull request Apr 13, 2021
Closed
@emyarod emyarod force-pushed the deprecate-controlled-password-input branch 2 times, most recently from c5e979e to f48d71e Compare April 28, 2021 15:43
@emyarod emyarod marked this pull request as ready for review April 28, 2021 16:06
@emyarod emyarod requested review from a team as code owners April 28, 2021 16:06
@emyarod emyarod force-pushed the deprecate-controlled-password-input branch from aaf2bb5 to 65ea019 Compare April 28, 2021 16:22
@joshblack
Copy link
Contributor

Is this one intended to be fully controlled/uncontrolled or is it more the more hybrid pattern where it has internal state and then if the prop passed in changes it updates?

@emyarod
Copy link
Member Author

emyarod commented Apr 29, 2021

the more hybrid pattern where it has internal state and then if the prop passed in changes it updates

the component should now follow this pattern

@emyarod emyarod force-pushed the deprecate-controlled-password-input branch from 26a858c to 83fc743 Compare April 29, 2021 15:20
@emyarod emyarod mentioned this pull request Apr 30, 2021
Merged
@emyarod emyarod force-pushed the deprecate-controlled-password-input branch from 280c47a to 41ec341 Compare May 12, 2021 16:47
@tw15egan
Copy link
Collaborator

Noticed a few issues:

Size variants are causing misalignment with the password icon
Screen Shot 2021-05-12 at 12 55 54 PM

Not seeing any focus styles on the password icon when using keyboard
Screen Shot 2021-05-12 at 12 56 03 PM

@emyarod
Copy link
Member Author

emyarod commented May 12, 2021

these are not regressions and should be addressed separately (the portions of code this PR modifies are unrelated)

@tw15egan
Copy link
Collaborator

Sounds good!

@emyarod emyarod force-pushed the deprecate-controlled-password-input branch from 1ff244a to aaaffe8 Compare May 13, 2021 15:23
@emyarod emyarod mentioned this pull request May 13, 2021
Merged
@kodiakhq kodiakhq bot merged commit 4a35ad5 into carbon-design-system:main May 13, 2021
@emyarod emyarod deleted the deprecate-controlled-password-input branch May 13, 2021 15:58
@tw15egan tw15egan mentioned this pull request May 13, 2021
Closed
22 tasks
@emyarod emyarod mentioned this pull request May 17, 2021
Closed
94 tasks
@tay1orjones tay1orjones mentioned this pull request Jan 25, 2024
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jnm2377 jnm2377 jnm2377 approved these changes

@tw15egan tw15egan tw15egan approved these changes

@andreancardona andreancardona Awaiting requested review from andreancardona

Assignees
No one assigned
Labels
status: ready to merge 🎉
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@emyarod @joshblack @tw15egan @jnm2377