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

Issue 3663 fix number input #3756

Merged
merged 6 commits into from
Mar 21, 2023

Conversation

cyantree
Copy link
Contributor

This fixes #3663 and #3729.

It's been quite a rework of the internal data handling so please have a closer look at it.

The key changes are:

  • Only propagate new values via onChange() when bluring the component or using the increment/decrement functionality.
  • Therefore while inputting some values nothing will be done to them.
  • When using it in controlled mode the value won't change at all and the displayed text will reset as soon as onChange() gets called
  • The value emitted in onChange() now already respects precision and min/max.
  • When setting a value that doesn't match the configuration (either because it's precision is higher than configured or it isn't within the min/max bounds) it will be displayed as is (so not rounded or clamped). However as soon as it has been changed by the user it gets rounded/clamped again.

@cyantree
Copy link
Contributor Author

I've uploaded a patched version of @mantine/core and integrated it into a sandbox to try out the new version:
https://codesandbox.io/s/mantine-numberinput-issues-2x16u7

This shows that the issues seem to be solved. However in combination with useForm() there's a glitch when blurring the input. Then for a frame the old value reappears again.
The is probably related to the fact that in a controlled mode the value gets restored then blurring the field because it's still the current one. And then the new value gets applied a tad later.

So that's something I have to look into.

@cyantree
Copy link
Contributor Author

I've fixed the glitch with useForm().

@rtivital
Copy link
Member

Only propagate new values via onChange() when bluring the component or using the increment/decrement functionality.

I'm not really sure that it is a good idea. It seems like a breaking change. I'll have a look at those issues. Most likely, we just need to turn off NumberInput formatting when it is focused.

@cyantree
Copy link
Contributor Author

cyantree commented Mar 14, 2023

It's like the native inputs change event also works.
For a native input If you want a per keystroke event you need to use the input event.

If you fire onChange() for every input, what should it output? IMHO it should always output the validated input as it otherwise would lead to issues in other places that depend on these values being clean.

So the newly required changes might be:

  • calling propagateNewValue() in handleChange()
  • reintroducing the focus logic from v5 that disables the useEffect() hook
  • Edit: It's a bit more complicated than that as it also affects other aspects.

@disillusionedowl
Copy link

we just need to turn off NumberInput formatting when it is focused.

IMHO that should be done in either case as it's pretty annoying UX-wise, try changing some numbers in the "Parser and formatter" section at https://mantine.dev/core/number-input/ It just feels off somehow, cursor and numbers jump around, etc.
I think in classic spreadsheet apps you also work with clean numbers during focus/edit.
It is prone to bugs due to much more complexity and possible edge cases.
I can't recall a use case where you would need this, it just looks like a nice-to-have eye candy only (downsides aside).

@rtivital
Copy link
Member

Fair point @disillusionedowl, thanks for sharing your thoughts. I think we will go with @cyantree solution then – onChange will not be called until input is blurred.

@disillusionedowl
Copy link

Great, thank you very much guys! The extra logic/options can always be added later behind some flags if there will be demand. The above looks like a solid default foundation.

@cyantree
Copy link
Contributor Author

Thank's! I agree that it would be great to have a working base again before adding/changing other things.

However honestly I don't understand the formatting issue that came up multiple times.
IMHO these are different things that can be resolved individually:

Aspect 1: Clean formatting during editing / Spreadsheet behaviour

That's a valid point that's currently partially implemented. I think we need two layers for formatting/parsing and three layers for data representation.

Consider the following example:
A user wants to input a currency amount in Euro.
So as a user based in Germany I would expect to enter the amount like 12000,34 (that's 12000 euro and 34 cents).
As a user in the USA I would expect to enter the amount like 12000.34 (that's also 12000 euro and 34 cents).

Then after blurring the input the amount would get formatted and the unit added.
So the DE based user might see 12.000,34 € whereas the USA based user might see €12,000.34

If the user then wants to edit the amount again the input field would show again 12000,34 (in DE) or 12000.34 (in US).

The internal value will always be 12000.34.

So the data representation layers are:

  1. Model data: 12000.34 (type number)
  2. Input data: Depending on the user/app locale. Something like 12000,34, 12000.34 (type string)
  3. Display data: Depending on the user/app locale. Something like 12.000,34 € , €12,000.34 (type string)

You need the following converters to work with these layers:

  • parse 2) to 1)
  • format 1) to 2)
  • format 2) to 3) or 1) to 3)

The current approach only has the data layers 1) and 3) and the formatter/parser converts between those two.

Aspect 2: Data flow handling via onChange() during editing

This describes what happens with data layer 1) while a user makes some edits.

The currently broken version triggers onChange() on every edit that can easily result in the update loop being triggered when it shouldn't. Therefore the component constantly refreshes, the cursor jumps and edits disappears.

To mitigate this I paused the data flow during editing.

It's still possible to trigger onChange() during edits but that has some implications:

  • During editing the component shouldn't process any value changes as this would again disrupt editing
  • onChange() should only output values that meet the configuration. So it should respect precision and clamping.

Therefore I think these two aspects can be discussed and improved separately. This PR only regarded Aspect 2 because that's where the issues came from.

@disillusionedowl
Copy link

You seem to have a good grasp on this @cyantree so it would be awesome, as you have possibly suggested, if you could continue with your work on this through other PRs, if you would be willing. Thanks again for this PR though, it's great as-is.

@rtivital
Copy link
Member

Thanks!

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.

Controlled NumberInput with precision buggy formatting
3 participants