Skip to content

Conversation

@martyphee
Copy link
Contributor

@martyphee martyphee commented May 20, 2016

Add support for updating the mask. Fixes #8.

Previously the cursor would go to the end of the input if you changed the mask or re-created the MaskedInput field. So, if you have a user typing 34 and then switching the mask to 1111 111111 11111 the cursor would be positioned to the end: 34__ ______ _____^.

This PR takes suggestions from PR #9.

@martyphee martyphee closed this May 20, 2016
@iamdustan
Copy link
Collaborator

what behavior is this intended to change or modify?

@martyphee martyphee reopened this May 20, 2016
@martyphee
Copy link
Contributor Author

@iamdustan I reopned this PR. Looks like the suggestions from the other issues/pr's fixed the issue.

@iamdustan
Copy link
Collaborator

cool. how extensively did you test this, btw?

@iamdustan
Copy link
Collaborator

could you update your commit message to describe the code change more clearly? E.g.

Add support for updating the `mask`. Fixes #8

@martyphee
Copy link
Contributor Author

cool. how extensively did you test this, btw?

Right now I'm testing our use case which is rebuilding our checkout pages. I can add it to other fields in the checkout flow.

@martyphee
Copy link
Contributor Author

@iamdustan Do you think we'll be able to get this merged on Monday?

@iamdustan
Copy link
Collaborator

Could you add a test case or two and an example of this to the demo?

That’ll make it easier to test out and verify it doesn’t regress in the future. Thanks!

@martyphee
Copy link
Contributor Author

Fair enough. Not sure how to do the test case. Need to see where the
cursor ends up I think.

On Sun, May 22, 2016 at 7:35 AM, Dustan Kasten notifications@github.com
wrote:

Could you add a test case or two and an example of this to the demo?

That’ll make it easier to test out and verify it doesn’t regress in the
future. Thanks!


You are receiving this because you modified the open/close state.
Reply to this email directly or view it on GitHub
#53 (comment)

@martyphee
Copy link
Contributor Author

I updated the demo and added a test. Let me know what you think.

On Sun, May 22, 2016 at 8:00 AM, Martin Phee martyphee@gmail.com wrote:

Fair enough. Not sure how to do the test case. Need to see where the
cursor ends up I think.

On Sun, May 22, 2016 at 7:35 AM, Dustan Kasten notifications@github.com
wrote:

Could you add a test case or two and an example of this to the demo?

That’ll make it easier to test out and verify it doesn’t regress in the
future. Thanks!


You are receiving this because you modified the open/close state.
Reply to this email directly or view it on GitHub
#53 (comment)

<MaskedInput
ref={(r) => {
if (r) ref = r
}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit on the spacing here.

@iamdustan
Copy link
Collaborator

awesome. Thanks for adding that demo and test case, @martyphee. I’m on PTO a lot this week so won’t be able to test it myself until next week. I’ve asked @jquense to give it a look at his earliest opportunity to get this shipped. Based on his work inside React’s DOM event handling I trust him more than me anyway :)

@martyphee
Copy link
Contributor Author

I fixed up the formatting. @jquense if you have time to review/merge/push that would be great. I have a outstanding PR in our project which depends on this change.

Thank you!

@jquense
Copy link
Collaborator

jquense commented May 24, 2016

LGTM, can you please squash the commits and change the message per: #53 (comment)

When resetting the mask the cursor would jump to the end of the line.  This PR fixes that behavior.
@martyphee
Copy link
Contributor Author

Done.

@jquense jquense merged commit 00511e5 into insin:master May 24, 2016
@martyphee martyphee deleted the fix-mask-changes branch May 24, 2016 13:45
@martyphee
Copy link
Contributor Author

@jquense When would you be able to do a release to npm? Thanks for the help.

@jquense
Copy link
Collaborator

jquense commented May 24, 2016

unfortunately no, I don't have publish rights to npm sorry!

@insin
Copy link
Owner

insin commented May 24, 2016

I just added monastic.panic as an npm owner - does anyone else need/want access?

@martyphee
Copy link
Contributor Author

@insin I only need it to get this published. I might contribute more in the future. martyphee

@jquense
Copy link
Collaborator

jquense commented May 24, 2016

ok published at 3.2.0

@martyphee
Copy link
Contributor Author

Awesome! Thank you!

On Tue, May 24, 2016 at 2:21 PM, Jason Quense notifications@github.com
wrote:

ok published at 3.2.0


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#53 (comment)

@iamdustan iamdustan mentioned this pull request May 26, 2016
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.

4 participants