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.

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

fix: text input mask fixes #2581

Merged

Conversation

nicholasguyett
Copy link
Contributor

Summary

Fixes the incorrect pass-through of the inputRef prop

Adds support for defaultValue

Adds support for using TextInputMask as a controlled component

Fixes support for externally set onChange hooks

Note: The maskString function contains the unmodified original masking implementation, just moved into a separate function so it can be called in both onChange and when setting the initial state.

Related Issues or PRs

How To Test

Manually tested this change in an external (currently private) application that uses both defaultValue and onChange.

Screenshots (optional)

@nicholasguyett nicholasguyett requested a review from a user September 6, 2023 17:30
@nicholasguyett nicholasguyett changed the title text input mask fixes fix: text input mask fixes Sep 6, 2023
@werdnanoslen werdnanoslen self-requested a review September 6, 2023 17:32
@rogeruiz
Copy link
Contributor

rogeruiz commented Sep 7, 2023

Thanks for the PR @nicholasguyett. I'll be taking a look at this and we will be incorporating in our next release. I'm currently looking through how to test this locally and appreciate your summary.

Copy link
Contributor

@rogeruiz rogeruiz left a comment

Choose a reason for hiding this comment

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

lgtm 🌈

I was able to test and build the project locally. Also, based on the How To Test section this looks like was already tested in a private repository.

Comment on lines +64 to +73
useEffect(() => {
// Make sure this component behaves correctly when used as a controlled component
setValue(
maskString(
((externalValue ?? defaultValue) as string) ?? ``,
mask,
charset
)
)
}, [externalValue])
Copy link
Contributor

Choose a reason for hiding this comment

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

Great work on this and using useEffect API hook.

Comment on lines +81 to +82
// Ensure the new value is available to upstream onChange listeners
e.target.value = newValue
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch here on ensuring other onChange listeners get the value here too.

@rogeruiz rogeruiz merged commit 84cf4d0 into trussworks:main Sep 8, 2023
7 of 8 checks passed
@brandonlenz
Copy link
Contributor

@all-contributors please add @nicholasguyett for bug, code

@allcontributors
Copy link
Contributor

@brandonlenz

We had trouble processing your request. Please try again later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants