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

modify componentWillReceiveProps to check with mask to see if mask va… #38

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mdgbayly
Copy link

…lue would be updated by new props rather than just checking to see if props have changes. Fixes issue where if a change is rejected by the client onChange handler, the component does not re-render.

See: insin/inputmask-core#16

mask: '111%'

Type: 9
MaskValue: 9%

Type: 9
MaskValue: 99%

Type: 9
MaskValue: 999%

But the value of 999 is rejected by onChange handler so component re-renders with props of 99 again. But the mask does not re-render as componentWillReceiveNewProps checks to see if props have changed and in this case have not.

…lue would be updated by new props rather than just checking to see if props have changes. Fixes issue where if a change is rejected by the client onChange handler, the component does not re-render
@alpjor
Copy link

alpjor commented Jun 14, 2016

This looks like a solid pull request to me. What's holding this back?

@iamdustan
Copy link
Collaborator

To follow up on this:

But the value of 999 is rejected by onChange handler so component re-renders with props of 99 again. But the mask does not re-render as componentWillReceiveNewProps checks to see if props have changed and in this case have not.

Is the end result that the Mask incorrectly renders 999% in this scenario?

More directly, does this implement a bug fix or an optimization?

@mdgbayly
Copy link
Author

@iamdustan Yes, the end result is that the mask incorrectly renders 999%.
So from my perspective its a bug fix.

Possibly this is related to how we are using the component. We are effectively using it as a controlled component.

Interestingly, I just upgraded our app to React 15, and I'm now getting warnings that MaskedInput needs to decide whether it is a controlled component or an uncontrolled component... Not sure if that is because our fork is behind and this has been remedied in the master origin, or whether this is a general problem with the implementation.

e.g.

MaskedInput contains an input of type undefined with both value and defaultValue props. Input elements must be either controlled or uncontrolled (specify either the value prop, or the defaultValue prop, but not both). Decide between using a controlled or uncontrolled input element and remove one of these props.

warning.js:44 Warning: MaskedInput is changing a controlled input of type undefined to be uncontrolled. Input elements should not switch from controlled to uncontrolled (or vice versa). Decide between using a controlled or uncontrolled input element for the lifetime of the component.

@mdgbayly
Copy link
Author

Actually, scratch my last comment about the warnings - I think that is a result of how we've integrated it. We're using it with React Redux Form, and I think that is where the duplicate defaultValue/value props are coming from.

@shkaper
Copy link

shkaper commented Jul 21, 2017

Would love to see this merged sometime

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