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

Material NumField with defaultValue cannot be cleared #969

Closed
KoenLav opened this issue May 20, 2021 · 4 comments · Fixed by #1000
Closed

Material NumField with defaultValue cannot be cleared #969

KoenLav opened this issue May 20, 2021 · 4 comments · Fixed by #1000
Assignees
Labels
Type: Bug Bug reports and their fixes
Milestone

Comments

@KoenLav
Copy link

KoenLav commented May 20, 2021

When using uniforms, simpl-schema (v2) and material, all at version 3.5.1 (latest version as of writing) when we clear a NumberField (in an AutoForm with an attached schema and model) the defaultValue will be set, whereas this works fine for a TextField.

Assuming the defaultValue of a NumberField is 10, when I backspace 0 it will show 1, but once I backspace 1 it will show 10 again.

Playground example

I believe these lines combined may be the culprit (setting the value to undefined instead of empty string will cause value to be set to initialValue):
https://github.com/vazco/uniforms/blob/master/packages/uniforms-material/src/NumField.tsx#L50
https://github.com/vazco/uniforms/blob/master/packages/uniforms/src/useField.tsx#L85

@radekmie radekmie self-assigned this May 22, 2021
@radekmie radekmie added the Type: Bug Bug reports and their fixes label May 22, 2021
@radekmie radekmie added this to the v3.x milestone May 22, 2021
@radekmie
Copy link
Contributor

Hi @KoenLav. That's a great finding! I'm actually surprised that no one has ever reported it earlier. When it comes to fixing that, it's a little problem. Changing the initialValue logic is a really important breaking change. The same goes for adjusting the NumFields of all themes (yes, it affects all themes).

I'm just thinking, what would be the best here. Maybe wrapping this code with a if (!changed) condition would be fine...? We'll have to check it.

@KoenLav
Copy link
Author

KoenLav commented May 23, 2021

Looking at the code: if changedMap contains all fields which no longer have their original value, then changed will only be false if we haven't modified a field, so !changed will only be trur when we have changed a field, in which case we don't want to set te value indeed.

@KoenLav
Copy link
Author

KoenLav commented May 23, 2021

However, I think initialValue should still be set (I don't think it matters much, but for consistency).

So maybe instead of initialValue = value, set initialValue using getInitialValue and only set value if !changed?

@radekmie
Copy link
Contributor

Well, that's not the changed/changedMap works. It stores the information on whether the value has ever changed, not whether it differs from the original one. It's still a breaking change though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Bug reports and their fixes
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants