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

[new component] auto-complete #2187

Merged
merged 2 commits into from Nov 21, 2015
Merged

Conversation

yongxu
Copy link
Contributor

@yongxu yongxu commented Nov 16, 2015

This component is the latest update from #1985

I decided to open a new PR because of the name change.

Known bug:
from @vijayrawatsan:

Lets say you type in 'h' - > 4 results
After that you type 'e' -> 1 result

No you press backspace to remove letter 'e'. Now the dropdown should show 4 results but it only show 1 result, rest is empty space in dropdown.

This is fixed when I turn animated: false while creating menu.

This might be caused by bug from src/menus/menu.jsx

@oliviertassinari the api has been changed to:

dataSource // for list proprity

/* function(searchText,prevDataSource) -> if return a list, 
   then replace dataSource with that list, 
   otherwise use the dataSource from props */
onUpdateInput 

//and
onNewRequest(searchText,index,dataSource) //if no match then index = null

Please merge, it should be ready! :)

return (
<ComponentDoc
name="Auto Complete"
componentInfo={[{
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd love to, just quite busy recently. I can do it this friday. I thought maybe we should push it first and then finish it? Since the component is usable, I wish more people can try it out before the document is ready.

@oliviertassinari oliviertassinari added Review: review-needed PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed Review: review-needed labels Nov 18, 2015
@yongxu
Copy link
Contributor Author

yongxu commented Nov 21, 2015

@oliviertassinari @shaurya947 It should be ready now :)

@@ -0,0 +1,45 @@
<AutoComplete
Copy link
Member

Choose a reason for hiding this comment

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

Could you remove the extra spaces?

@oliviertassinari
Copy link
Member

@yongxu The API looks good to me 👍. Could you have a look at my comment? I will merge then.

non-unidirectional data flow pattern is removed, in order to fit flux
model.
Now dataSource accept object {key:component} pattern
Improved performance.
oliviertassinari added a commit that referenced this pull request Nov 21, 2015
[AutoComplete] Add this new component
@oliviertassinari oliviertassinari merged commit 5423492 into mui:master Nov 21, 2015
@oliviertassinari
Copy link
Member

@yongxu Thanks for your effort!

@yongxu
Copy link
Contributor Author

yongxu commented Nov 21, 2015

Thanks for all the review @oliviertassinari 👍

@vijayrawatsan
Copy link

@yongxu @oliviertassinari @shaurya947 👍
Finally its merged.

@shaurya947
Copy link
Contributor

Yes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants