-
-
Notifications
You must be signed in to change notification settings - Fork 533
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
[2.0] Unify input components APIs #5732
Conversation
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. |
Now that the plan is to just go for 2.0, I was able to refactor the The |
|
* Centralize initialization lifecycle * Leave new Before/After methods on input base component * Fix the method calls * Adjust DatePicker, NumericEdit, and Select * Use custom compare function * Comments
# Conflicts: # Source/Blazorise/Components/DatePicker/DatePicker.razor.cs # Source/Blazorise/Components/Radio/Radio.razor.cs # Source/Extensions/Blazorise.DataGrid/_DataGridCellDateEdit.razor
# Conflicts: # Documentation/Blazorise.Docs/Models/Snippets.generated.cs # Documentation/Blazorise.Docs/Pages/Docs/Extensions/DataGrid/Code/DataGridFilterModeColumnTemplateFilteringExampleCode.html # Documentation/Blazorise.Docs/Pages/Docs/Extensions/DataGrid/Examples/DataGridFilterModeColumnTemplateFilteringExample.razor # Source/Blazorise/Components/ColorPicker/ColorPicker.razor.cs # Source/Blazorise/Components/DatePicker/DatePicker.razor.cs # Source/Blazorise/Components/TimePicker/TimePicker.razor.cs # Source/Extensions/Blazorise.DataGrid/_DataGridCellDatePicker.razor # Source/Extensions/Blazorise.DataGrid/_DataGridCellFilter.razor # Source/Extensions/Blazorise.DataGrid/_DataGridMenuFilter.razor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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 withSelect
andDatePicker
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
, andIReadOnlyList<TValue> SelectedValues
parameters. On their own they are not problematic. But to make them work under the sameSelect
component, historically, I had to inherit fromBaseInputComponent
withTValue
defined asIReadOnlyList<TValue>
.SelectedValue
internally, we just force it as an array of one item.SelectedValues
internally, we just use the array as it isAnd there is our problem. Since BaseInputComponent inherits as
IReadOnlyList<TValue>
, our newValue
parameter is also defined asIReadOnlyList<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>
, andBaseInputComponent<TValue>
.BaseInputComponent<string>
I had good results initially but then I had many problems with casting from and to string for Guid, Bool, and other types.BaseInputComponent<TValue>
I also made much progress. Until I figured that inMultiple
mode nothing works. If we binded TValue as an array, none of theSelectItem
TValue
would match theTValue
type on the parentSelect
.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
, andSelectedValues
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.