Locale Inputs, Resolving broken embroiderer dependencies, Unfocus Formatting#541
Locale Inputs, Resolving broken embroiderer dependencies, Unfocus Formatting#541ErvinSabic wants to merge 26 commits intoCrowdStrike:mainfrom
Conversation
… and updated test app
Hotfix ts
…l number formatting
|
|
Hey @ErvinSabic, thanks for working on this! Just kicked off CI. Tests including Embroider are passing! 🎉 But some type errors, fyi |
|
Yep! Still working on it. o7 |
…k on local number input
…n multiple languages
|
@simonihmig I encourage you to give this a try locally. I'm going to be writing more tests for it and I want to try it on a mobile device as well to see what happens with the caret positioning. It should work for: Does not support: It is worth noting that there are edge cases with the way Intl.NumberFormat may handle some things. For example, in a Saudi Arabian Locale if you enter "2" in a input that is setup for units, and the units being liters, that locale actually has a specific word that communicates "2 liters". Just based on my testing. I imagine it's much like how an American might call "12 eggs" a "dozen eggs". Though I'm not a linguist and I don't speak arabic. I'm stopping here because if I keep chasing edge cases this will never get done and in its current form supports the majority of use cases one might expect out of a component like this. |
ErvinSabic
changed the title
simonihmig
left a comment
simonihmig
left a comment
There was a problem hiding this comment.
@ErvinSabic thanks a lot for your effort, and sorry again for the late feedback!
I think it would really help if we could split this PR into the three different issues/features it tackles!
- the dependency updates are really good and uncontroversial, so would be nice to get them in asap
- for the number input issue #468, I left a comment here
- the new locale-number component seems quite massive, I would need to play with that locally. It seems to do much more than fixing #483, like adding unit support, right? This is interesting, but really a completely new feature. Even just for changelog generation, it would help to have this extracted. On the #483 issue though, I left some comments there. There seems to be a misunderstanding as the native number input does correctly handle localized numbers (at least for me), and you cannot opt-out of localized rendering. On my machine it will always render with a comma as the decimal separator. But also as mentioned there, that assumed
parseFloatissue might not even be an issue.
| this.args.setValue( | ||
| this.type === 'number' ? parseFloat(e.target.value) : e.target.value | ||
| ); | ||
| this.args.setValue(e.target.value); |
There was a problem hiding this comment.
Hm, I don't think we can do that. This would mean when inputting, the value is set as a string, but when focussing out as a number. The user might not see the difference in the UI, but the internal value would be flip-flopping between string and number. This could have effects on the validation (if used). Like when you have a custom validator or using yup for example, the validation result might be different based on whether the value is a string or a number!
There was a problem hiding this comment.
Wouldn't parsing as a number in here cause the original issue (#468) because the user isn't done typing?
There was a problem hiding this comment.
In this comment I was suggesting to just not call setValue, when the (string) value is in that kinda interim state where parseFloat wouldn't produce a new number.
Something like
const valueAsNumber = parseFloat(e.target.value);
if (valueAsNumber !== this.args.value) {
this.args.setValue(valueAsNumber);
}
Now when typing 2.01...
parseFloat('2') ->setValue(2)`- `parseFloat('2.') is still 2, so same value, don't call setValue
- `parseFloat('2.0') is still 2, so same value, don't call setValue
parseFloat('2.01') is 2.01 ->setValue(2.01)`
Does that make sense?
There was a problem hiding this comment.
Gotcha. Let me break this PR up into their respective issues and work on it further.
2 participants
This seeks to solve a few issues:
#468
#483
Dependency issue mentioned by Windvis here - #540 (comment)
Will mark as ready when I believe it can be submitted for review.