Skip to content

Conversation

@digitaldirk
Copy link
Member

Now when strict is false the selected item will be scrolled to when opening the Autocomplete.

Description

When selecting an item with strict set to false, then clicking the Autocomplete the list would just open to the first item. This behavior is the opposite of what MudSelect does. This PR matches the behavior and will scroll to selected item.

Do we want to add a bool parameter that turns the auto scroll on/off?

(You can compare the current behavior: select an item, then reopen the select/autocomplete)
https://www.mudblazor.com/components/select#visual-playground
https://www.mudblazor.com/components/autocomplete#strict-mode

How Has This Been Tested?

Visually tested with current tests passing.

Type of Changes

Could be a breaking change if user implemented custom scroll logic?

Before:
beforescroll
After:
afterscroll

Checklist

  • The PR is submitted to the correct branch (dev).
  • My code follows the code style of this project.
  • I've added relevant tests.

Match behavior to MudSelect
@Anu6is
Copy link
Contributor

Anu6is commented Mar 18, 2025

I'm not sure that this would be enough. Remember that the AutoComplete, by default, shows only the first 10 items. So, if the user selects the 12th item in the list by typing, when they re-open the list the selected item won't be rendered in the initial 10 items meaning it can't be scrolled to.

@digitaldirk
Copy link
Member Author

@Anu6is Very true I will need to adjust this. I have all my autocompletes with null for max items.

Thanks

Add logic to account for MaxItems being not null
@digitaldirk
Copy link
Member Author

With latest commit this now accounts for non-null MaxItems. The logic gets a range of MaxItems based on the index of the seleected value.

This example is the using the default of 10. Washington has 3 states under it so in this case, 6 states are above it (3 + selected + 6 = 10). But when in the middle of the list, there would be 5 states below the selected state, and 4 above (for a total of 10 states).
MaxItemsFix

Account for MaxItems > total # of items
@digitaldirk
Copy link
Member Author

I will fix the current failing tests and add test(s) for the new functionality but first I would appreciate feedback before I do that.

Do we want to be able to toggle on/off this functionality? I can see some not wanting this, but I think it should be on be default because that is the way MudSelect works.

@digitaldirk
Copy link
Member Author

Sorry for the pings but I'd like feedback before continuing to work on this.

@henon @danielchalmers @versile2

Thank you for your time :)

@versile2
Copy link
Contributor

versile2 commented Apr 8, 2025

I can't speak for others but I like auto scrolling to it and I don't see a need to add yet another option. There's no harm in it and I can't think of a scenario where scrolling automatically to the selected item would cause an issue. I do know there is a pending PR to fix some functionality so I would wait until that is merged then pull it into this one before continuing. Ultimately the decision falls to others but that is my opinion.

V

@henon
Copy link
Collaborator

henon commented Apr 8, 2025

I am for adding this too. I would even go so far as to call the current behavior a bug.

@danielchalmers
Copy link
Member

Good thinking about compatibility concerns, but I agree this is a bug and can be fixed without any extra steps

New scroll to logic now shows different items/different item indexes depending on what is searched for. Changed tests to now correctly find indexes/expected items.
Combine if statement
@digitaldirk
Copy link
Member Author

Thank you all for the feedback @Anu6is @versile2 @henon @danielchalmers

Fixed the failing tests and the SonarQube Major issue, but could use some help on:

  1. https://sonarcloud.io/project/issues?issueStatuses=OPEN%2CCONFIRMED&pullRequest=11049&id=MudBlazor_MudBlazor&open=AZWq4zV_GkTDPuUTQLzl&tab=code This one is going over my head - could I get an explanation or help on fixing this one? Why is it complaining about using Value != null - we use that on https://github.com/digitaldirk/MudBlazor/blob/a427d59ddf6b47bd5bd6a288374fbf16d7ce88db/src/MudBlazor/Components/Autocomplete/MudAutocomplete.razor.cs#L703 as well.
  2. Sorry about the formatting and many commits (on a work network with firewalls so I have to edit the commits raw in the browser 😓) Can someone format my changes?

All done besides any action on the above 2 points

@versile2
Copy link
Contributor

versile2 commented Apr 9, 2025

Thank you all for the feedback @Anu6is @versile2 @henon @danielchalmers

Fixed the failing tests and the SonarQube Major issue, but could use some help on:

1. https://sonarcloud.io/project/issues?issueStatuses=OPEN%2CCONFIRMED&pullRequest=11049&id=MudBlazor_MudBlazor&open=AZWq4zV_GkTDPuUTQLzl&tab=code This one is going over my head - could I get an explanation or help on fixing this one? Why is it complaining about using Value != null - we use that on https://github.com/digitaldirk/MudBlazor/blob/a427d59ddf6b47bd5bd6a288374fbf16d7ce88db/src/MudBlazor/Components/Autocomplete/MudAutocomplete.razor.cs#L703 as well.

2. Sorry about the formatting and many commits (on a work network with firewalls so I have to edit the commits raw in the browser 😓) Can someone format my changes?

All done besides any action on the above 2 points

  1. because it's a generic type, generally just putting default does the trick. I'm not good at explaining though so if someone else wants to please do.
  2. You will have to fix alignment and spacing issues. Do you not have access at home to fix? Only core members can "edit" your PR and they have way too much to do. As for commits make as many as you like we only look at the end result.

@versile2
Copy link
Contributor

Are you able to update the spacing issues and what not?

@digitaldirk
Copy link
Member Author

Are you able to update the spacing issues and what not?

Yes, will be able to do the last steps this week.

@versile2
Copy link
Contributor

Are you able to update the spacing issues and what not?

Yes, will be able to do the last steps this week.

Awesome, I think this is a value bug fix, hit me up on Discord dm if you have any questions.

V

@codecov
Copy link

codecov bot commented Apr 24, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.10%. Comparing base (17f3e3e) to head (3b50172).
Report is 22 commits behind head on dev.

Additional details and impacted files
@@           Coverage Diff           @@
##              dev   #11049   +/-   ##
=======================================
  Coverage   91.10%   91.10%           
=======================================
  Files         465      465           
  Lines       14407    14407           
  Branches     2788     2788           
=======================================
  Hits        13126    13126           
  Misses        642      642           
  Partials      639      639           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@digitaldirk digitaldirk marked this pull request as draft April 25, 2025 04:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Request for adding a new feature or enhancing existing functionality (not fixing a defect)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants