-
-
Notifications
You must be signed in to change notification settings - Fork 510
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
New SelectIndexedList Component #3317
base: master
Are you sure you want to change the base?
New SelectIndexedList Component #3317
Conversation
Seems good, although I would like it better if it was part of an existing component. I will try to make work on SelectList in the next few days, my schedule is full at the moment so I cannot properly test it. Thanks for the PR, btw 💪 |
The problem with
This is my first try at adding the SelectedIndex to the SelectList. Seems to be working fine but more tests are welcome on your side. Test <Button Color="Color.Primary" Clicked="@(()=>{ selectedItem = items[1]; return Task.CompletedTask; })">SELECT ITEM</Button>
<Button Color="Color.Danger" Clicked="@(()=>{ items.RemoveAt(1); return Task.CompletedTask; })">REMOVE ONE</Button>
<SelectList TItem="Item" TValue="int" Data="items" @bind-SelectedItem="@selectedItem" ValueField="@((item)=>item.Id)" TextField="@((item)=>item.Name)" />
<Div>
Selected item: @selectedItem?.Name
</Div>
@code {
private IList<Item> items = new List<Item>()
{
new Item() { Id = 1, Name = "Value 1" },
new Item() { Id = 2, Name = "Value 2" },
new Item() { Id = 3, Name = "Value 3" },
new Item() { Id = 4, Name = "Value 4" },
new Item() { Id = 5, Name = "Value 5" },
};
private Item? selectedItem;
internal class Item
{
public int Id { get; set; }
public string Name { get; set; }
}
} |
if ( !selectedValue.Equals( value ) ) | ||
{ | ||
selectedValue = value; | ||
SelectedValueChanged.InvokeAsync( selectedValue ).ConfigureAwait( false ); |
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.
You must not call InvokeAsync in synchronous code.
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.
I had a look at asp.net core components and the pattern they use is to trigger changed events use within a internal CurrentValueProperty. I propose to the same as Property can be changed from different sources. And there is no other way then raising InvokeAsync in synch code. At least I use no _ to indicate that code does not care of the task output.
#endregion | ||
|
||
#region Methods | ||
|
||
protected Task HandleSelectedValueChanged( TValue value ) | ||
{ | ||
SelectedValue = value; | ||
return SelectedValueChanged.InvokeAsync( value ); |
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.
Any reason why this is removed?
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.
As changes also can be trigger from SelectedItem change, the raising of the events shall when the property changes.
/// Returns true if SelectedItem property is supported. | ||
/// </summary> | ||
public bool SelectedItemSupported => | ||
ValueField == null && Data is IList && typeof(TValue) == typeof(int); |
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.
Why support only typeof(int)
? I think practically any TValue
type should be indexed.
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.
In a IList, the index is always int. A standard collection supporting other types as index would be a dictionary which is not assignable from IEnumerable. We would need to move to object for Data (btw as WPF/UWP does with ItemSource)
Wow, fast response! First, I agree with some of your comments that's why I tried to merge the features into SelectList. To avoid breaking changes, the solution is not pretty but at least we have a bindable SelectedItem property and Validation works as well changes of the Data collection. I have tested it successfully with the following scenarios: <SelectList TItem="Item" TValue="int" Data="itemsList" @bind-SelectedItem="@selectedItem" TextField="@((item) => item.Name)">
@*selectedItem = Item, selectedValue = index and default Item *@
<SelectList TItem="Item" TValue="int" Data="itemsList" @bind-SelectedItem="@selectedItem3" TextField="@((item)=>item.Name)" DefaultItemText="make your choice" DefaultItemValue="-1" />
@*selectedValue = index *@
<SelectList TItem="Item" TValue="int" Data="itemsList" @bind-SelectedValue="@selectedValue" TextField="@((item)=>item.Name)" />
@* previous behavior with ValueField *@
<SelectList TItem="Item" TValue="int" Data="itemsList" @bind-SelectedValue="@selectedValue2" TextField="@((item)=>item.Name)" ValueField="@((item)=>item.Id)" /> |
@David-Moreira can you try looking at this if you have time? |
I'll take a look over these days.
Does Edit: |
SelectList works with values and texts, same as DropdownLis,t for example. With this PR we could bind to an actual element instead of just the value. |
Ah great! Yea no point in doing a new component. We should also update the other one's if they do not support it. |
Hello @lolochristen , 1- If I mutate
2- If I have two
Finally here's the full code that I was going to start testing that's based on your examples, before I found these issues:
|
I would say the reason for the error is that |
@lolochristen just a friendly reminder. :) Understand if you're busy, of course. |
@stsrki Let's resume this for 1.3? Also, some time has passed, I'm having a hard time remembering how this component is different then a regular |
As far as I remember, this PR introduces an internal List that holds the Items, and then we can access them by the Index. Then again, I was thinking we should probably go forward with making something similar on the regular |
I see. Let's investigate and we might abandon this one. |
The components Select and SelectList are not able to directly return and bind to an item from the assigned list. I created a new component with this capability as the existing SelectList would have most likely experienced a breaking change. the new component uses the index of a IList for the SelectItem and synchronizes with SelectedItem property.
Please consider this capability as I at was a crucial missing feature in my projects. Feel free to integrate directly or replace SelectList.
Example: