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

number-field: inconsistencies when onChange and onRawValueChange is called #418

Open
jcmonnin opened this issue May 17, 2024 · 0 comments
Open

Comments

@jcmonnin
Copy link

jcmonnin commented May 17, 2024

In text-field, onChange is only called when change originates from user interaction, but not when the controlled value changes. I think this is the right semantics. Some use cases need to be able to make the distinction if change comes from user action.

See this playground to illustrate the text-field behavior. Note that on the console, no onChange message appears when Set Text to "Test" button is pressed:
https://playground.solidjs.com/anonymous/174c66df-0efe-4815-9469-abc6f0ecc552

number-field doesn't behave that way. Here are the examples:

  1. Controlled with text: https://playground.solidjs.com/anonymous/e72d7529-1ec1-43f6-903d-0bdefa35e4e1
  2. Controlled with number: https://playground.solidjs.com/anonymous/99c481fe-e885-40a4-a643-aebe8e99c3f8

In case 1) note that when inputting a number bigger than 1000, and then pressing Set Number to 10 it first calls onChange with the formatted value having the thousand separator, which shouldn't be called when value is discarded when controlled value changes.

Side Note: In practice the text mode isn't really suitable as I don't have access to formatter/parser from the outside and it's necessary to convert numbers. Note that as soon as value is formatted with separators, controlled value is wrong.

Case 2) is the one that matters in practice. At construction, onRawValueChange is called with NaN, which is debatable if it should be called. While typing the number bigger than 1000 onRawValueChange is called as expected. However when the Set Number to 10 button is pressed, onRawValueChange is called twice, once again with the typed number (after formatting) and then with 10. If wanting to reproduce text-field semantics, it should not be called at all since it just takes the externally controlled value.


I add some slightly subjective opinion about number-field here:

  1. There is following API to specify controlled value value: Accessor<string | number | undefined> and rawValue: Accessor<number>. It's not clear why two props are needed and if using always value is equivalent or not. What happens if two separate signals are passed? Maybe it's better to have a single source of truth for the controlled value (eg. remove rawValue prop).

  2. I think it's hard to have the correct behavior in edge cases when having controlled values for both text and number. In react spectrum NumberField, they have chose to just expose the number, which I think would be sufficient. I rather have a simpler API with the event semantics like text-field rather than more flexibility but unpredictable calls to events.

  3. When text of number field is empty, I prefer having undefined in the onRawValueChange callback, rather than NaN. The main reason I prefer to avoid it is the NaN != NaN behavior which can be nasty in change detection (in signals or other places). Note that this is quite subjective.

  4. Naming: I was initially a bit confused by the naming of rawValue. Given this component is about number, my brain associates value with the number and raw with the text form as typed. It's obviously not a big issue, especially since the documentation is great. I would find the inverse naming more logical, or alternatively some name that avoids raw like numberValue.

@jcmonnin jcmonnin changed the title number-field: inconsistentcies when onChange and onRawValueChange is called number-field: inconsistencies when onChange and onRawValueChange is called May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 📋 To do
Development

No branches or pull requests

1 participant