Skip to content

Infra/masked input to ts #1976

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
Merged
merged 11 commits into from
May 8, 2022
Merged

Infra/masked input to ts #1976

merged 11 commits into from
May 8, 2022

Conversation

ethanshar
Copy link
Collaborator

Description

Remove usage of legacy TextField from MaskedInput and migrate component to TS and function component

Changelog

Refactor MaskedInput component, remove legacy TextField usage

@ethanshar ethanshar requested a review from M-i-k-e-l April 13, 2022 14:37
M-i-k-e-l
M-i-k-e-l reviewed Apr 14, 2022
View reviewed changes
Copy link
Collaborator

@M-i-k-e-l M-i-k-e-l left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Consider moving the screen to TS as well.
Also, the screen did not work as expected for me, typing more than one char in the first example did not always work.

@ethanshar ethanshar requested a review from M-i-k-e-l April 19, 2022 08:06
M-i-k-e-l
M-i-k-e-l reviewed Apr 19, 2022
View reviewed changes
Copy link
Collaborator

@M-i-k-e-l M-i-k-e-l left a comment

Choose a reason for hiding this comment

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

I could not comment on older conversations so I had to re-create them :/

@M-i-k-e-l
Copy link
Collaborator

Consider moving the screen to TS as well.

I think you might have missed this.

Also, the screen did not work as expected for me, typing more than one char in the first example did not always work.

I think that using the computer's keyboard is the issue, using the simulator \ Android works.

@ethanshar
Copy link
Collaborator Author

@M-i-k-e-l
I fixed the PR comments
I also changed a little the "new" implementation after playing with in the private components.
First, I turned it into an uncontrolled component because of how the formatter (used to be called transformer) used to work. So I also added an initialValue prop

@ethanshar ethanshar requested a review from M-i-k-e-l April 19, 2022 14:26
M-i-k-e-l
M-i-k-e-l reviewed Apr 20, 2022
View reviewed changes
src/components/maskedInput/new.tsx
* @gif: https://camo.githubusercontent.com/61eedb65e968845d5eac713dcd21a69691571fb1/68747470733a2f2f6d656469612e67697068792e636f6d2f6d656469612f4b5a5a7446666f486f454b334b2f67697068792e676966
* @example: https://github.com/wix/react-native-ui-lib/blob/master/demo/src/screens/componentScreens/MaskedInputScreen.js
*/
function MaskedInput(props: MaskedInputProps, ref: ForwardedRef<any>) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've written this in the main PR area, they should make that more noticeable somehow...

Consider moving the screen to TS as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Definitely. Fixed and also add a new cool example (:

M-i-k-e-l
M-i-k-e-l reviewed Apr 20, 2022
View reviewed changes
M-i-k-e-l
M-i-k-e-l reviewed Apr 20, 2022
View reviewed changes
@ethanshar ethanshar requested a review from M-i-k-e-l April 27, 2022 14:57
M-i-k-e-l
M-i-k-e-l reviewed Apr 28, 2022
View reviewed changes
demo/src/screens/componentScreens/MaskedInputScreen.tsx Outdated
<View row centerH>
{_.times(4, i => {
return (
<View marginR-s3 center style={styles.pinCodeSquare}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing key

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

right, fixed!

demo/src/screens/componentScreens/MaskedInputScreen.tsx Outdated
<MaskedInput
migrate
maxLength={4}
ref={this.priceInput}
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should create a dedicated ref, currently only one of the two users (price \ pin) are deleted.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed as well (:

@ethanshar ethanshar requested a review from M-i-k-e-l May 8, 2022 05:24
@M-i-k-e-l M-i-k-e-l merged commit bd45ce7 into master May 8, 2022
@ethanshar ethanshar deleted the infra/MaskedInput_toTS branch April 2, 2024 06:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@M-i-k-e-l M-i-k-e-l M-i-k-e-l approved these changes

Assignees

@M-i-k-e-l M-i-k-e-l

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ethanshar @M-i-k-e-l