Skip to content

Conversation

@ZeeshanTamboli
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli commented Dec 25, 2024

Reverts #41300 for v6.

Re-opens #40843 which I'll try to solve in a separate PR by introducing a new prop.

@ZeeshanTamboli ZeeshanTamboli added package: material-ui scope: autocomplete Changes related to the autocomplete. This includes ComboBox. labels Dec 25, 2024
@mui-bot
Copy link

mui-bot commented Dec 25, 2024

Netlify deploy preview

https://deploy-preview-44858--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 0c70ca0

@ZeeshanTamboli ZeeshanTamboli merged commit 6ab5bcb into mui:master Dec 25, 2024
21 checks passed
@ZeeshanTamboli ZeeshanTamboli deleted the revert-41300-v6.x branch December 25, 2024 15:07
@DiegoAndai
Copy link
Member

@ZeeshanTamboli may I ask you to explain what's impact this change has on users? It's not clear to me. I'm trying to understand if we can release #44916 as a patch or if we have to do a minor bump.

@ZeeshanTamboli
Copy link
Member Author

ZeeshanTamboli commented Jan 3, 2025

@DiegoAndai This reverts to the previous behavior to fix #44048 for v6, though it also brings back #40843, as mentioned in the PR description. I'll address #40843 separately, possibly by introducing a new prop, as it's less critical than #44048. A regression is worse than a bug.

Basically I followed #44266 (comment).

Since #44048 is a v5 issue and a separate PR (#44857) reverted it in v5, perhaps we should include a v5 release in this cycle as well?

@ZeeshanTamboli ZeeshanTamboli mentioned this pull request Jan 3, 2025
@DiegoAndai
Copy link
Member

DiegoAndai commented Jan 3, 2025

This reverts to the previous behavior to fix #44048 for v6, though it also brings back #40843, as mentioned in the PR description

Basically I followed #44266 (comment).

@ZeeshanTamboli, this still doesn't explain the impact on users. I know this PR is a revert. The original PR gives very little explanation for the impact on users either. Even though a PR is a revert, we should explain what the end result is for users. So, again, what will change on the user's end after this change?

@ZeeshanTamboli
Copy link
Member Author

This reverts to the previous behavior to fix #44048 for v6, though it also brings back #40843, as mentioned in the PR description
Basically I followed #44266 (comment).

@ZeeshanTamboli, this still doesn't explain the impact on users. I know this PR is a revert. The original PR gives very little explanation for the impact on users either. Even though a PR is a revert, we should explain what the end result is for users. So, again, what will change on the user's end after this change?

@DiegoAndai With this revert, the Autocomplete popper/dropdown will remain in the DOM (hidden when open is false), even if options is empty ([]). This allows users to pass custom Popper/Paper components.

Previously: The dropdown wasn't rendered in the DOM when options was empty, not allowing users to pass custom Popper/Paper component even if there are no options. We relied on whether options were empty or available.
Now (with this revert): The dropdown stays in the DOM but relies on the open state to determine visibility.

I hope this helps.

@DiegoAndai
Copy link
Member

DiegoAndai commented Jan 3, 2025

Thanks @ZeeshanTamboli, this explains it 👍🏼.

I'm only releasing v6 today to test another bug fix, but I'll remember to include a v5 release in next week's cycle 👍🏼

@ibu-tv
Copy link

ibu-tv commented Jan 9, 2025

Hi @DiegoAndai , looking forward to see this in the v5 release. :)

@aarongarciah
Copy link
Member

@ibu-tv the corresponding change (#44857) was released this week in v5.16.14

@ibu-tv
Copy link

ibu-tv commented Jan 16, 2025

@aarongarciah , @aarongarciah , @DiegoAndai - thanks for work on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

scope: autocomplete Changes related to the autocomplete. This includes ComboBox.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants