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

[2.0] Unify input components APIs #5732

Open
wants to merge 24 commits into
base: next-2.0
Choose a base branch
from
Open

Conversation

stsrki
Copy link
Collaborator

@stsrki stsrki commented Sep 13, 2024

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>.

  • For SelectedValue internally, we just force it as an array of one item.
  • For SelectedValues internally, we just use the array as it is

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>.

  • With 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.
  • With BaseInputComponent<TValue> I also made much progress. Until I figured that in Multiple mode nothing works. If we binded TValue as an array, none of the SelectItem TValue would match the TValue type on the parent Select.

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
Copy link
Collaborator Author

stsrki commented Sep 16, 2024

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 stsrki added this to the 2.0 milestone Sep 18, 2024
@stsrki stsrki changed the title Unify input components APIs [2.0] Unify input components APIs Sep 18, 2024
@stsrki
Copy link
Collaborator Author

stsrki commented Sep 18, 2024

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
Copy link
Collaborator Author

stsrki commented Sep 18, 2024

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 stsrki changed the base branch from master to next-2.0 September 23, 2024 10:09
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.

[2.0] Unify input component APIs
1 participant