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

[Autocomplete] Warn when using wrong getOptionSelected #19699

Merged
merged 4 commits into from
Feb 20, 2020
Merged

[Autocomplete] Warn when using wrong getOptionSelected #19699

merged 4 commits into from
Feb 20, 2020

Conversation

ahmad-reza619
Copy link
Contributor

@ahmad-reza619 ahmad-reza619 commented Feb 14, 2020

Close #19595

I followed what suggested in the issue, but i suggest to change the props getOptionSelected to be more precise. It can be as what suggested by @oliviertassinari like optionEqualValue, or my recommendation is getSelectedOption. but it's up to you guys (actually i had a little hard time thinking what this props does, but i don't know if it's will become a breaking change 🤔 )

i used array filter here. it does support all kinds of browsers except IE 6-8 based on this report

Have a nice day 😄

@mui-pr-bot
Copy link

mui-pr-bot commented Feb 14, 2020

No bundle size changes comparing 5ed8d0a...0bd00e0

Generated by 🚫 dangerJS against 0bd00e0

@oliviertassinari
Copy link
Member

@ahmad-reza619 Thanks, it would be perfect with a test case.

@oliviertassinari oliviertassinari changed the title [Autocomplete] warn when using wrong implementation of getOptionSelected [Autocomplete] Warn when using wrong getOptionSelected Feb 14, 2020
@oliviertassinari oliviertassinari added the component: autocomplete This is the name of the generic UI component, not the React module! label Feb 14, 2020
@ahmad-reza619
Copy link
Contributor Author

@oliviertassinari alright, will do

@ahmad-reza619
Copy link
Contributor Author

ahmad-reza619 commented Feb 14, 2020

I'm a bit confused as to where to put the test case, Autocomplete or useAutoComplete? 🤔 sorry btw i'm new when it comes to testing

@oliviertassinari oliviertassinari added new feature New feature or request PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Feb 14, 2020
@oliviertassinari
Copy link
Member

@ahmad-reza619 Do you think that you could add a test case? :)

@ahmad-reza619
Copy link
Contributor Author

@ahmad-reza619 Do you think that you could add a test case? :)

i'll do but it's kinda hard for me 😢 i don't understand about testing and this lib surely huge😅 i would like some guidance 😄

@ahmad-reza619
Copy link
Contributor Author

do i have to only give getOptionSelected props in testing 🤔 and also am i have to use stub or spy to replicate it's functionality. 😓 sorry btw still confused here

@eps1lon
Copy link
Member

eps1lon commented Feb 18, 2020

do i have to only give getOptionSelected props in testing and also am i have to use stub or spy to replicate it's functionality. sorry btw still confused here

Try writing a component that passes props to Autocomplete so that this warning is triggered. For how to assert on the warning you could use https://github.com/mui-org/material-ui/blob/f57707d1c328d22e2ce46dd12f08164271534f90/packages/material-ui/src/Breadcrumbs/Breadcrumbs.test.js#L84-L110 as an inspiration.

@ahmad-reza619
Copy link
Contributor Author

@eps1lon but this one a little bit complex i think 🤔 because you have to make a component to assert this behavior. and there's no example that i can use 😓

@oliviertassinari oliviertassinari self-assigned this Feb 19, 2020
@oliviertassinari oliviertassinari removed the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Feb 19, 2020
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Don't worry, writing a test case can be challenging. It requires a deep understanding of what's going on. I have added one, I think that we can move forward :)

@ahmad-reza619
Copy link
Contributor Author

Don't worry, writing a test case can be challenging. It requires a deep understanding of what's going on. I have added one, I think that we can move forward :)

Thank you so much for the help, but it's fail? is it because of the test 🤔 ?

@oliviertassinari oliviertassinari merged commit a5a3785 into mui:master Feb 20, 2020
@oliviertassinari
Copy link
Member

Azure pipeline has been unreliable since yesterday, I don't know if the issue is on our side, there are out of memory crashes. Reruning solves the problem. To monitor. Thanks.

@OmarZidan1997
Copy link

@oliviertassinari
I faced the same error here and im using the example in material ui documentation.. Is there any way i can fix that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: autocomplete This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Autocomplete] Warn wrong comparison getOptionSelected implementation
5 participants