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

MudSlider no longer accepts nullable types #8860

Closed
1 of 2 tasks
danielchalmers opened this issue May 3, 2024 · 10 comments · Fixed by #8881
Closed
1 of 2 tasks

MudSlider no longer accepts nullable types #8860

danielchalmers opened this issue May 3, 2024 · 10 comments · Fixed by #8881

Comments

@danielchalmers
Copy link
Contributor

danielchalmers commented May 3, 2024

Bug type

Component

Component name

MudSlider

What happened?

I have a form which the user fills out that tracks the amount of sleep they got which is stored in a decimal?.

A null state indicates that the user has not made a selection yet. If this was a non-nullable decimal then you would not be able to tell the difference between 0 meaning no choice and 0 meaning no sleep.

Expected behavior

MudSlider should be able to handle nullable types again like

  • MudToggleGroup
  • MudCheckBox
  • MudRating
  • MudNumericField
  • MudTextField
  • MudRadioGroup

Reproduction link

https://try.mudblazor.com/snippet/QaGSETaHbgFnzVTr

Reproduction steps

  1. Create a decimal? property
  2. Bind it to MudSlider value
  3. Compile

Relevant log output

No response

Version (bug)

7.0.0-preview1

Version (working)

6.19.1

What browsers are you seeing the problem on?

Edge

On which operating systems are you experiencing the issue?

Windows

Pull Request

  • I would like to do a Pull Request

Code of Conduct

  • I agree to follow this project's Code of Conduct
@danielchalmers
Copy link
Contributor Author

@ScarletKuro We talked about this on Discord and I wanted to get it tracked here for discussion

@danielchalmers
Copy link
Contributor Author

@henon What do you think? I'm using this in a production app which is why I made the issue.

@henon
Copy link
Collaborator

henon commented May 4, 2024

I think this is an accidental break and the easiest way would be to revert to using the numeric converter. It is true, we support nullable types in all inputs, so the slider shouldn't be an exception to that

@ScarletKuro
Copy link
Member

ScarletKuro commented May 4, 2024

I think this is an accidental break and the easiest way would be to revert to using the numeric converter. It is true, we support nullable types in all inputs, so the slider shouldn't be an exception to that

Personally to me, that the component doesn't override the setters / getters with the converters anymore overweight the nullable feature (which makes the component to be "stable" and correctly designed) considering that there is a reasonable workaround available to understand that the value wasn't selected and personally to me it seems that the component author didn't even intend the nullable usage as the slider position is inadequate when the the value is null and depending on the max it can be in between the middle and the end and it's more like a coincidence that it can work with it, and probably only few people exploit this.
But that's only my view.

@henon
Copy link
Collaborator

henon commented May 4, 2024

It is a good question where the slider knob should be in case of null. Slider has no way of visualizing the value-not-set-state or null-state.

Can we somehow find a compromise? Like a way to define the not-set value (i.e. -1 or -infinity or whatever)? Maybe Value can be made nullable again but we'd need a non-nullable replacement for Value, like NumericValue which takes its place for the sake of the advantages we gained with Kuros changes.

@ScarletKuro
Copy link
Member

ScarletKuro commented May 4, 2024

It is a good question where the slider knob should be in case of null. Slider has no way of visualizing the value-not-set-state or null-state.

I know that angular has the knob in the begging when the value is null.
but for example in MUI it's not possible to have no value in the slider, if will always default to zero when value is null.
But it's anyway awkward to compare it with JS frameworks since the language is not strongly typed and they have an easy way to implement this compared to the C#

Like a way to define the not-set value (i.e. -1 or -infinity or whatever)?

That's what I suggested as a workaround and meant under "reasonable workaround" if you set the initial value less that your min, lets say -1 value and min 0, the knob will be in beginning and once you start moving know value will be positive and never reach the negative value again.

@ScarletKuro
Copy link
Member

ScarletKuro commented May 4, 2024

FluentUI uses generic math as well as far as I can see.
Also it's hard to take slider as an full-fledged input component, it's not part of the MudBaseInput, it's not part of the MudFormComponent, there is no readonly, disable properties etc.

MudRating

But SelectedValue doesn't have a nullable support, in fact it doesn't use generics at all, it's using a compromise of combination of zero value and HandleHoveredValueChanged

@danielchalmers
Copy link
Contributor Author

@ScarletKuro

full-fledged input component

Do you think it's a good time to make it a MudFormComponent so it supports that stuff? I would gladly work on that so it fits with the others in Form & Inputs.

MudRating

You're right, I checked and I did that binding a different way.

@ScarletKuro
Copy link
Member

ScarletKuro commented May 4, 2024

Do you think it's a good time to make it a MudFormComponent so it supports that stuff?

My answer that it should like in Radzen, but the time is not good. Not unless MudFormComponent is rewritten as there is 100 and 1 things incorrectly done in this component and I do not want to introduce another unstable component as we are trying to do the opposite for now. Component stability > any amount of features. Once MudBlazor core is matured and gets rid of side effects with paramters setters / getters, writing in parameters, not awaiting async then we can polish this things.

@henon
Copy link
Collaborator

henon commented May 5, 2024

Introducing a T NumericValue parameter and a T NullValue could solve this problem. T? Value would make a null check and update NumericValue accordingly. Internally only NumericValue would be used for calculations etc.

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 a pull request may close this issue.

3 participants