Megabit / Blazorise

Blazorise is a component library built on top of Blazor with support for CSS frameworks like Bootstrap, Tailwind, Bulma, AntDesign, and Material.
https://blazorise.com/
Other
3.32k stars 535 forks source link

[2.0] Unify input components APIs #5732

Closed stsrki closed 2 days ago

stsrki commented 2 months ago

Closes #5596

Most of the reasoning for this PR is already in the original issue. Here, I will explain what is good and what is not with the approach.

The good part is that most of the components were quite easy to convert. TextEdit, DateEdit, NumberEdit and such, all now use the same Value parameter.

While initially, I had a good start with making all input components to use Value parameter, I stumbled on the blocker with Select and DatePicker components. All components except those two have a single parameter to bind values. I will describe the problem with Select only. DatePicker is practically the same.


So. Select have TValue SelectedValue, and IReadOnlyList<TValue> SelectedValues parameters. On their own they are not problematic. But to make them work under the same Select component, historically, I had to inherit from BaseInputComponent with TValue defined as IReadOnlyList<TValue>.

And there is our problem. Since BaseInputComponent inherits as IReadOnlyList<TValue>, our new Value parameter is also defined as IReadOnlyList<TValue> type. So using the Select component would mean that Value must be an array type, even for single select mode.

I tried everything to work around it. Tried to inherit from BaseInputComponent<string>, and BaseInputComponent<TValue>.

Basically, anything I tried, we would introduce a breaking change in 1.7. With current approach I left TValue as an array type. And also left SelectedValue, and SelectedValues without making them obsolete. We could go with this for 1.7, and then for 2.0 I can revisit the work. By then, we don't need to fear breaking changes, which should be somewhat easier to refactor.

stsrki commented 2 months ago

Considering the limitations in this version, the best approach is to do this in 2.0. It will be a temporary solution even if we mark all current APIs as obsolete. So, it is best not to introduce too many changes for something that will not be used much in production. I will continue the work on this as it is 2.0. Once I am done, I will leave it until we start working on 2.0.

My plan is for 1.7 to be the last version 1.x. Then, focus only on 2.0 and API cleanup. With this PR, it will be one of the first changes.

stsrki commented 2 months ago

Now that the plan is to just go for 2.0, I was able to refactor the Select component. It now handles all binding through the Value, and ValueChanged parameters. There is no need for multiple SelectedValue, and SelectedValues.

The Value parameter supports single value types, as well as array or list value types. Whatever is bound to the Value will be appropriately handled.

stsrki commented 2 months ago

Now that the plan is to just go for 2.0, I was able to refactor the Select component. It now handles all binding through the Value, and ValueChanged parameters. There is no need for multiple SelectedValue, and SelectedValues parameters.

The Value parameter supports single value types, as well as array or list value types. Whatever is bound to the Value will be appropriately handled.

stsrki commented 2 days ago

Hard to review alot of file changes. I guess the main changes reside in BaseInput, which looks fine to me. I really like the new OnBefore and OnAfter. Since it's such a wide PR, I'd merge this as soon as possible so we end up testing it while developing other features.

Thanks for the review. I agree, it is best to merge and then continue with any work left from here.