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

NumericInput: fix manual input boundaries check #1745

Merged
merged 5 commits into from
Jun 4, 2021

Conversation

IIARROWS
Copy link
Sponsor Contributor

@IIARROWS IIARROWS commented Jun 1, 2021

This PR fixes an issue with NumericField, an out of bound value doesn't change on the interface when manually typed in, but it's bound value is costrained.

How to reproduce
https://try.mudblazor.com/snippet/wYQlOKOlWPefTgtG
This fiddler replicates the "Normal vs Immediate vs Debounced" example in the docs, but with Min=0 Max=10 parameters.
It's easier seen on immediate, but it happens on debounced and normal (in some cases).
If you type a valid number (IE: 4) you can see change immediately both on the textbox and in the control below. If you make it invalid (IE: 40) the control value changes to 10 (maximum valid value) but the text is still 40.

Why it happens
NumericField declared a DefaultConverter<string> (forcing InvariantCulture to work with HTML standard specifications) on the internal MudInput component, that deals with the number but it doesn't care if the number is valid so it isn't changed.

How it's fixed
Created a NumericBoundariesConverter<T> where T must be a number format, but it's a string => string converter. It holds a reference to a function that returns T given a parsed T.
The OnGet function parses the input string, evaluates the value before converting it back to a string, allowing to make evaluations on the numeric value beyond the simple "is valid".
Inside NumericField constructor, it binds the evaluation function to MudNumericField.ConstrainBoundaries in order to check the boundaries with the current Min/Max values.

@henon
Copy link
Collaborator

henon commented Jun 3, 2021

Can you also add a test case that directly tests the new converter similar to the ones for DefaultConverter and Co?

@IIARROWS
Copy link
Sponsor Contributor Author

IIARROWS commented Jun 4, 2021

Add three tests: one checks the input/output, another the results of the function and a third for the formatting.

@henon
Copy link
Collaborator

henon commented Jun 4, 2021

@JonBunator Yeah, the [Testcase] tag is handy when you know about it. I did not know about it for a long time and only learned about it late during this project when Rene Sackers used it for a test. Had I known, I would have used it for many of the tests I wrote and it would have saved me a shitload of time.

But already written test cases don't need to be changed. They fulfill their duty well enough.

@henon henon removed the needs tests label Jun 4, 2021
@henon henon merged commit bb895c7 into MudBlazor:dev Jun 4, 2021
@henon
Copy link
Collaborator

henon commented Jun 4, 2021

Thanks @IIARROWS for taking the time to write tests!

@henon henon added this to the 5.0.11 milestone Jun 4, 2021
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.

None yet

3 participants