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

[FormsyText] Add updateImmediately prop & fix handleChange #109

Merged
merged 14 commits into from
Sep 14, 2016

Conversation

mbrookes
Copy link
Collaborator

@mbrookes mbrookes commented Jun 6, 2016

I believe this resolves the various issues for both use cases. Let me know!

This is based in part on and supersedes @codaholicguy's PR: #103.

If you use onValid / onInvalid to control the submit button, and the last form element before the submit button is a text-field, you should use updateImmediately on that field to provide an error as to why the form can't be submitted without the user having to remove focus from that field.

I increased the timeout slightly, as otherwise the error can come in while still typing for a slower typist.

If you don't control the submit button (or the textfield isn't the last value before the button), the default behaviour is now consistent. onValid / onInvalidupdate immediately, but the error only appears when focus shifts.

Closes #97.

@mbrookes mbrookes force-pushed the formsytext-fix-error-on-update branch from 4708bf1 to 5affb7d Compare June 6, 2016 22:09
Accepts Formsy.Form.reset()
@vkbrad
Copy link

vkbrad commented Jun 9, 2016

👍

reebalazs and others added 2 commits July 11, 2016 17:25
- Make sure that both value and defaultValue can be provided.
- Make sure that changing the props and resetting the form works.
- Also fix reset to work, when value and defaultValue are not specified.
- Only update the pristine value, but not the actual value, in case props.defaultValue has changed.
- Add tests for value and defaultValue props interactions.
- Remove previous componentWillReceiveProps which targeted to fix reset.
Also, update peer dependency
Fix text input values, and form reset
@leMaik
Copy link

leMaik commented Jul 12, 2016

Works great for me so far, but I think that throttle is better in this case (difference between throttle and debounce). Otherwise, the form doesn't get re-validated while typing (at rather fast speed) but only after typing.

@jayalfredprufrock
Copy link
Contributor

Agreed that throttle seems to make more sense for this use case. I'd love to see this pull request get merged as I've been dealing with the "last text input in a form not enabling the submit button" issue for quite some time and it really does cause confusion for some users. Is there something I can do to help?

@codeaholicguy
Copy link
Collaborator

I see. Throttle seems to be better for this case. What is status of this pull request?

@mbrookes
Copy link
Collaborator Author

@codeaholicguy It needs a rebase after recent merges. I can see pros and cons to throttle vs. debounce, but I'm not yet convinced that throttle would be preferable. I'm open to feedback as to why. With throttle, a slower typist is going to get an error when their going-to-be-valid, but as yet incomplete entry hits the throttle timeout.

With debounce, a fast typist doesnt get an error until they're done, but if they're a fast typist, they can fix thier error quickly. 😄

@mbrookes
Copy link
Collaborator Author

@jayalfredprufrock

Is there something I can do to help?

When we agree on the best approach, you're welcome to take this PR, fix it up and resubmit it.

@jayalfredprufrock
Copy link
Contributor

@mbrookes I hear ya, and I could certainly be happy with either.

I think the reason this is a tough call is we have two use cases for the updateImmediately prop:

  1. Notifying the user that their input is invalid without requiring them to move to the next field. In this case debounce is preferable to prevent bombarding the user with error messages before they've been given a chance to finish typing.
  2. Notifying the user immediately when their input is valid. In this case, throttle is preferable so the user gets notified as soon as possible, eg. validation errors get cleared, submit buttons enabled, etc.

If I had to choose one, the latter seems more useful. My guess is accomplishing both effects would require some non-trivial internal changes, but perhaps we could provide the two versions separately via two different props?? So updateImmediately would provide a throttled version and another prop, say updateWhenIdle, would provide a debounced version?

@mbrookes
Copy link
Collaborator Author

Having the choice via a prop would be good, but what should the default be? 😄

@teknologist
Copy link

Hi,

Any news on this one ?

Many thanks.

@codeaholicguy
Copy link
Collaborator

@mbrookes can I take responsibility of rebasing this PR?

remote: Permission to mbrookes/formsy-material-ui.git denied to codeaholicguy.
fatal: unable to access 'https://github.com/mbrookes/formsy-material-ui.git/': The requested URL returned error: 403

@mbrookes
Copy link
Collaborator Author

@codeaholicguy Yes please!!!

@codeaholicguy
Copy link
Collaborator

@mbrookes I rebased

@mbrookes
Copy link
Collaborator Author

mbrookes commented Sep 14, 2016

Thanks @codeaholicguy!

I'm merging this as is, as I don't have the cycles to review right now, but since this is mostly based on your code, I know it's good!

I think it would be good to get community feedback before we release, so lets hold back on pushing to npm for a bit and see if this throws up any issues.

BTW, I gave you write access to the repo, so feel free to make other changes, or merge good PRs.

@codeaholicguy
Copy link
Collaborator

@mbrookes thank you! Should I merge this PR or wait for you?

@mbrookes mbrookes merged commit ff14bc3 into master Sep 14, 2016
@mbrookes
Copy link
Collaborator Author

mbrookes commented Sep 14, 2016

Oops! In my haste I forgot to confirm the merge. Done now.

@codeaholicguy codeaholicguy deleted the formsytext-fix-error-on-update branch October 4, 2016 06:44
@codeaholicguy codeaholicguy mentioned this pull request Oct 7, 2016
@ryanblakeley
Copy link
Collaborator

I'm using this updateImmediately option and getting a new error in the browser console.

Unknown prop `updateImmediately` on <input> tag. Remove this prop from the element. For details, see https://fb.me/react-unknown-prop

@codeaholicguy
Copy link
Collaborator

@rojobuffalo currently, it hasnt been tagged yet, so please use formsy-material-ui direct from github source

@valoricDe
Copy link

Hi, thanks for this PR. Could you explain why you added a debounce? @jayalfredprufrock Could you explain how a user gets bombarded by error messages? Either the input is already invalid, so the message is already shown under the textfield or the input gets invalid, and as soon as this happens I want to know about and not wait for 400ms.

@jayalfredprufrock
Copy link
Contributor

jayalfredprufrock commented Oct 24, 2016

The debounce behavior becomes desirable when you have an input that is guaranteed to fail validation at the first key stroke. Consider a password confirmation field or an email field. It's a little distracting to see the flash of an error message telling you your password is incorrect or email invalid when you just haven't had a chance to finish typing. In a perfect world, the user would be notified immediately when the field becomes valid, but only notified of invalid input after a short idle period.

Earlier I was suggesting some kind of "updateWhenIdle" type functionality where the field can only be invalidated after it has received focus and at least one character, and the user hasn't typed any thing in 400ms. Haven't had a chance to look at the above implementation to know how things are supposed to work now, but it sounds like either way this is a nice improvement to what we had before.

@valoricDe
Copy link

@jayalfredprufrock I like the ideal world ^^, maybe following merge request goes into this direction: #155

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

Successfully merging this pull request may close these issues.

Typing in FormsyText doesn't update Form validity