Uh oh!
There was an error while loading. Please reload this page.
feat(input): add input-password-toggle component - #29175
Merged
Merged
Conversation
This reverts commit de678b5.
liamdebeasi
commented
Mar 19, 2024
Uh oh!
There was an error while loading. Please reload this page.
liamdebeasi
commented
Mar 19, 2024
Uh oh!
There was an error while loading. Please reload this page.
liamdebeasi
commented
Mar 19, 2024
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
liamdebeasi
commented
Mar 20, 2024
Uh oh!
There was an error while loading. Please reload this page.
2 tasks
liamdebeasi added a commit
that referenced
this pull request
Mar 21, 2024
Issue number: Internal --------- <!-- Please do not submit updates to dependencies unless it fixes an issue. --> <!-- Please try to limit your pull request to one type (bugfix, feature, etc). Submit multiple pull requests if needed. --> ## What is the current behavior? <!-- Please describe the current behavior that you are modifying. --> [Amanda pointed out that the ripple effect for the Button inside of InputPasswordToggle was not working](#29175 (comment)). I found out that calling `ev.preventDefault` on `pointerdown` causes `mouseup` to not get fired. On desktop, we rely on `mouseup` to know when to add the ripple effect. (`touchend` is not impacted) Interestingly, calling `ev.preventDefault` on `pointerdown` does **not** prevent `pointerup` from being fired. The idea here is that if we migrate the tap click utility to use the PointerEvents API instead of separate mouse/touch listeners we can keep the existing tap click behavior while also fixing the bug that Amanda noted. ## What is the new behavior? <!-- Please describe the behavior or changes that are being added by this PR. --> - Tap click nows listens for the Pointer Events instead of separate mouse/touch events Impact to developers is fairly minimal. There should be no behavior change (other than the bug I noted being fixed). There should be a very small perf boost because this util now only adds 4 event listeners on the document instead of 7 previously. ## Does this introduce a breaking change? - [ ] Yes - [x] No <!-- If this introduces a breaking change: 1. Describe the impact and migration path for existing applications below. 2. Update the BREAKING.md file with the breaking change. 3. Add "BREAKING CHANGE: [...]" to the commit description when merging. See https://github.com/ionic-team/ionic-framework/blob/main/.github/CONTRIBUTING.md#footer for more information. --> ## Other information <!-- Any other information that is important to this PR such as screenshots of how the component looks before and after the change. --> Reviewers: Please manually test this on desktop devices as well as iOS and Android devices (not Chrome Dev Tools. iOS simulators are fine). Test that components such as `ion-button` correctly add the activated state (or ripple effect for MD). Also verify that the activated state is not added when tapping the button and then scrolling. For desktop, check that right clicking does not add the activated state.
averyrousseau
approved these changes
Mar 21, 2024
averyrousseau
left a comment
Contributor
There was a problem hiding this comment.
We discussed the possibility of using the new round button styles in #29161 instead of needing a shadow part to style the new button, but I don't see that as a blocker 👍
Uh oh!
There was an error while loading. Please reload this page.
liamdebeasi
commented
Mar 22, 2024
ContributorAuthor
There was a problem hiding this comment.
This is correct. The team made this screenshot update in next but it never made its way into feature-8.0
brandyscarney
approved these changes
Mar 22, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue number: Internal
What is the current behavior?
When given a password input it is hard to know what users are typing as the contents of the input are obscured. As a result, it is a common pattern to have a button that lets users temporarily toggle the visibility of the password so they can correct any mistakes. Ionic currently has the infrastructure for developers to implement this on their own, but this use case is so common that the team thinks it is worth having this functionality built-in to Ionic.
What is the new behavior?
ion-input-password-togglecomponent. This component is a button that toggles the visibility of the text in the input it is slotted into.Does this introduce a breaking change?
Other information
Docs PR: ionic-team/ionic-docs#3541
Note: We did not do the approach listed in the other PR due to #29141 (comment).