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.

Sign up for GitHub

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

Jump to bottom

[Autocomplete] Render endAdornment only when necessary #32386

Merged
merged 6 commits into from
May 4, 2022
Merged

[Autocomplete] Render endAdornment only when necessary #32386

merged 6 commits into from
May 4, 2022

Conversation

g1eny0ung
Copy link
Contributor

@g1eny0ung g1eny0ung commented Apr 20, 2022 •
edited

Signed-off-by: Yue Yang g1enyy0ung@gmail.com

As the title. If there has no hasClearIcon or hasPopupIcon specified, its no need to render the endAdornment.

@g1eny0ung
chore: render endAdornment conditionally
08ac1f2 
Signed-off-by: Yue Yang <g1enyy0ung@gmail.com>
@mui-bot
Copy link

mui-bot commented Apr 20, 2022 •
edited

Details of bundle changes

Generated by 🚫 dangerJS against f31d249

@danilo-leal danilo-leal added the component: autocomplete This is the name of the generic UI component, not the React module! label Apr 20, 2022
@mnajdova mnajdova changed the title [Autocomplete] Render endAdornment conditionally [Autocomplete] Render endAdornment only when necessary Apr 22, 2022
mnajdova
mnajdova reviewed Apr 22, 2022
View reviewed changes
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes makes sense to me. Please add a test case for this new behavior.

@mnajdova mnajdova added the PR: needs test The pull request can't be merged label Apr 22, 2022
@g1eny0ung
Copy link
Contributor Author

Changes makes sense to me. Please add a test case for this new behavior.

Got it. I will add it soon.

@g1eny0ung
test: should only render endAdornment
d01fd59 
Signed-off-by: Yue Yang <g1enyy0ung@gmail.com>
@g1eny0ung
fix: typo
1f88d6a 
Signed-off-by: Yue Yang <g1enyy0ung@gmail.com>
@g1eny0ung
fix(ci): format code
8f24594 
Signed-off-by: Yue Yang <g1enyy0ung@gmail.com>
@g1eny0ung
Copy link
Contributor Author

@mnajdova Already added. Most of the cases where endAdornment isn't rendered are when freeSolo is defined, so I only added the test under prop: freeSolo.

PTAL when you're free. 😃

@g1eny0ung g1eny0ung requested a review from mnajdova April 25, 2022 10:25
@ZeeshanTamboli ZeeshanTamboli removed the PR: needs test The pull request can't be merged label Apr 27, 2022
@g1eny0ung
chore: address comments
f4c3d87 
Signed-off-by: Yue Yang <g1enyy0ung@gmail.com>
@ZeeshanTamboli
Copy link
Member

@g1eny0ung It's a great first pull request on MUI 👌🏻. Thank you for working on it!

@g1eny0ung
Copy link
Contributor Author

@ZeeshanTamboli It's my pleasure. Thanks for your careful correction! 😃

@ZeeshanTamboli ZeeshanTamboli merged commit 8a0c68c into mui:master May 4, 2022
@g1eny0ung g1eny0ung deleted the chore/autocomplete-endAdornment branch May 4, 2022 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZeeshanTamboli ZeeshanTamboli ZeeshanTamboli approved these changes

@mnajdova mnajdova Awaiting requested review from mnajdova

Assignees
No one assigned
Labels
component: autocomplete This is the name of the generic UI component, not the React module!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@g1eny0ung @mui-bot @ZeeshanTamboli @mnajdova @danilo-leal