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

[Lists]: don't make parameters only be set once #3457

Merged
merged 2 commits into from
Mar 3, 2025

Conversation

joriverm
Copy link
Contributor

@joriverm joriverm commented Feb 28, 2025

Pull Request

📖 Description

as described in #3416, the parameters in the list controls can no longer be updated from outside the control at the moment (selected option(s) being the one we use). this was done to fix #3297 but the boolean checks seem to not be needed?

🎫 Issues

Fix #3416, Fix #3297

👩‍💻 Reviewer Notes

ive taken the example from the original issue and expanded it with the buttons to set or clear the combobox.
Im setting this as a Draft because i have no idea if this is actually a good fix and like to get some feedback if possible.
also FluentCombobox_ClearSelection test is failing now, which i find weird since it should now be able to clear :/
nvm, test was broken

📑 Test Plan

@page "/FluentComboBox"

@using System.Linq.Expressions

<Form Model="modelTest" EnableFluentValidation=true>
    <FluentCombobox
            SelectedOptionChanged="@((selectedOption) => OnSelectedOptionChangedAsync(selectedOption))"
            ValueChanged="@((value) => OnValueChangedAsync(value))"
            ValueExpression="@(() => modelTest.SelectedValue)"
            TOption="OptionItem"
            Items="@Items"
            OptionValue="@(i => i.Value)"
            OptionText="@(i => i.Name)"
            OptionDisabled="@(i => i.Disabled)"
            OptionSelected="@(i => i.Value == modelTest.SelectedValue)"
            Autocomplete=ComboboxAutocomplete.Both
            Placeholder="Testing binding" />

    <FluentButton OnClick=@(() => { modelTest.SelectedValue = null; })>Clear</FluentButton>
    <FluentButton OnClick=@(() => { modelTest.SelectedValue = Items.First().Value; })>Select First</FluentButton>
    <p>@modelTest.SelectedValue</p>
</Form>

@code {
    private OptionItem[] Items { get; set; } = [
        new OptionItem { Name = "Test 1", Value = "TEST1", Disabled = false },
        new OptionItem { Name = "Test 2", Value = "TEST2", Disabled = false },
        new OptionItem { Name = "Test 3", Value = "TEST3", Disabled = false }
    ];

    public class ModelTest
    {
        public string? SelectedValue { get; set; }
    }

    private ModelTest modelTest = new();

    private async Task OnSelectedOptionChangedAsync(OptionItem selectedOption)
    {
        Console.WriteLine($"OnSelectedOptionChangedAsync enter modelTest.SelectedValue:{modelTest.SelectedValue}, selectedOption.Value:{selectedOption.Value}.");
        if (modelTest.SelectedValue != selectedOption.Value)
        {
            Console.WriteLine($"OnSelectedOptionChangedAsync before modelTest.SelectedValue:{modelTest.SelectedValue}, selectedOption.Value:{selectedOption.Value}.");
            modelTest.SelectedValue = selectedOption.Value;
            Console.WriteLine($"OnSelectedOptionChangedAsync after modelTest.SelectedValue:{modelTest.SelectedValue}, selectedOption.Value:{selectedOption.Value}.");
        }

        await Task.CompletedTask;
    }

    private async Task OnValueChangedAsync(string? text)
    {
        Console.WriteLine($"OnValueChangedAsync enter modelTest.SelectedValue:{modelTest.SelectedValue}, text:{text}.");
        if (modelTest.SelectedValue != default 
            && (string.IsNullOrEmpty(text) || !Items.Select(i => i.Name).Contains(text)))
        {
            Console.WriteLine($"OnValueChangedAsync before value:{modelTest.SelectedValue}");
            modelTest.SelectedValue = default;
            Console.WriteLine($"OnValueChangedAsync after value:{modelTest.SelectedValue}");
        }

        await Task.CompletedTask;
    }

    public class OptionItem
    {
        public string? Name { get; set; }
        public string? Value { get; set; }
        public bool Hidden { get; set; }
        public bool Disabled { get; set; }
    }
}

✅ Checklist

General

  • I have added tests for my changes.
  • I have tested my changes.
  • I have updated the project documentation to reflect my changes.
  • I have read the CONTRIBUTING documentation and followed the standards for this project.

Component-specific

  • I have added a new component
  • I have added Unit Tests for my new component
  • I have modified an existing component
  • I have validated the Unit Tests for an existing component

⏭ Next Steps

N/A

@joriverm
Copy link
Contributor Author

@vnbaaij : sorry about that, i mustve missed something in the html

@dvoituron
Copy link
Collaborator

These changes seem to revert to the code introduced in PR #3300.
Have you checked whether this PR is still correct?

@joriverm
Copy link
Contributor Author

joriverm commented Feb 28, 2025

These changes seem to revert to the code introduced in PR #3300. Have you checked whether this PR is still correct?

it reverts part of it, the internal value stuff is kept in place, normally. problem is that the boolean check makes it not update the value when selectedoption (or even value) would be changed because it doesn't execute the whole selection changed code :/ ( as shown by the test i had to edit/fix)
this is also why i made it a draft. something is up and i dont know what...

but with the given example, from previous issue, it didn't break again so i have no idea if the example is wrong, im testing wrong, or this fixes everything :/

@vnbaaij vnbaaij marked this pull request as ready for review March 3, 2025 22:40
@vnbaaij vnbaaij changed the title fixes: [Lists]: don't make parameters only be set once [Lists]: don't make parameters only be set once Mar 3, 2025
@vnbaaij vnbaaij merged commit 03fb1a9 into microsoft:dev Mar 3, 2025
4 checks passed
@vnbaaij vnbaaij added this to the v4.11.6 milestone Mar 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants