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] Migrate to use popover #2634

Merged
merged 1 commit into from
Dec 23, 2015

Conversation

alitaheri
Copy link
Member

I'm still figuring out how the auto-complete works and why it uses instance properties instead of state.
Closes #2632 and #2595
@oliviertassinari Please check this out. It is still buggy, and sometimes forcefully holds the focus. I know it's coming from the callback passed to onBlur event of TextField, without it, it looks ugly. With it sometimes it won't leave the focus. play with the component for a while and you might be able to reproduce it (for me it's fairly easy)

@yongxu could use your help again, if you've got the time 😁

@alitaheri
Copy link
Member Author

@oliviertassinari @newoga This is more important that theme. we need this for 0.14.0 😁 Those can wait :P

@oliviertassinari
Copy link
Member

Sure, the theme question should really be answered but can wait a bit (it can have a lot of good impacts).

I have noticed one change, the width of the Popover is different.
https://github.com/callemall/material-ui/blob/master/src/DropDownMenu/DropDownMenu.jsx#L325 could help.

@alitaheri
Copy link
Member Author

check again please 😁

@oliviertassinari
Copy link
Member

@subjectix Yes, that's better 👍. I haven't any other bugs.

@alitaheri
Copy link
Member Author

@oliviertassinari Thanks for reviewing this 😁 Can I merge after a squash? ^_^

@alitaheri alitaheri force-pushed the auto-complete-use-popover branch from 08b79bf to f38b954 Compare December 23, 2015 21:15
@newoga
Copy link
Contributor

newoga commented Dec 23, 2015

@subjectix @oliviertassinari I agree that the theme stuff can wait. Sorry for getting caught up in it. I haven't reviewed this (but I don't think I need to). The changes seem sound and I'm happy with it. 😄

@alitaheri
Copy link
Member Author

@newoga Thanks ^.^ I myself have some issues with it, but they relate to how popover works. We should improve the API and possibly the behavior. It's a very great component, but it can be improved a lot.

@newoga
Copy link
Contributor

newoga commented Dec 23, 2015

I agree! @yongxu's initial PR was the first time I contributed (via commenting) to this project. So I also have him to thank for getting me involved. 😄

@newoga
Copy link
Contributor

newoga commented Dec 23, 2015

I'm hoping when I get around #2416 I can also incorporate AutoComplete behavior from this.

@alitaheri
Copy link
Member Author

@newoga Yeah #2416 is a great change. really awesome 👍 Really looking forward to see it come to reality 👍 👍

@oliviertassinari
Copy link
Member

Yes, let's merge this. Sometimes we have to take risks and break stuff to move fast.
@subjectix Thanks

oliviertassinari added a commit that referenced this pull request Dec 23, 2015
[WIP] [AutoComplete] Migrate to use popover
@oliviertassinari oliviertassinari merged commit 4fd977e into mui:master Dec 23, 2015
@alitaheri alitaheri deleted the auto-complete-use-popover branch December 23, 2015 22:02
@oliviertassinari
Copy link
Member

was the first time I contributed

@newoga We are glad you are here 👍

@alitaheri
Copy link
Member Author

@oliviertassinari Thanks, sorry my code was a bit sloppy. We should fix a lot of stuff in 0.14.1 😁

@alitaheri
Copy link
Member Author

@newoga We are glad you are here 👍

Yeah you've been a great help so far 👍 👍

@newoga
Copy link
Contributor

newoga commented Dec 23, 2015

Thanks guys, I'm glad to be here! You both (and many others I've seen while perusing the issues as I often do in many projects) have been great examples of how to build an inviting community 👏 .

@alitaheri
Copy link
Member Author

@newoga Glad you like this community, I love it myself ❤️ ❤️

@alitaheri alitaheri changed the title [WIP] [AutoComplete] Migrate to use popover [AutoComplete] Migrate to use popover Dec 24, 2015
@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work component: autocomplete This is the name of the generic UI component, not the React module! labels Jan 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: autocomplete This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dialog body hides Autocomplete dropdown
3 participants