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] Fix the regressions #3858

Merged
merged 1 commit into from
Apr 2, 2016
Merged

[AutoComplete] Fix the regressions #3858

merged 1 commit into from
Apr 2, 2016

Conversation

oliviertassinari
Copy link
Member

Closes #3854.

@oliviertassinari oliviertassinari added this to the 0.15.0-beta.1 Release milestone Apr 2, 2016
@oliviertassinari oliviertassinari merged commit cbf5e3b into mui:master Apr 2, 2016
@oliviertassinari oliviertassinari deleted the autocomplete-fix-closing branch April 2, 2016 13:09
@oliviertassinari
Copy link
Member Author

There is nothing to review, the fixes are really simple.

@mbrookes
Copy link
Member

mbrookes commented Apr 2, 2016

There is nothing to review

Yes there is. 😄

This changes the signature of getStyles(). The rest of the components have getStyles(props, context, state).

@oliviertassinari
Copy link
Member Author

Oh, I have merged too quickly 😱.
Shouldn't this be this opposite? Eg, the spec of shouldComponentUpdate is (nextProps, nextState, nextContext).

@mbrookes
Copy link
Member

mbrookes commented Apr 2, 2016

So what happened is:

Originally the components had getStyles(props, state).

When we removed the default theme, it became getStyles(props, context)

This was enough for most components, but some needed state in getMuiTheme() for other state than muiTheme, so state got appended togetMuiTheme()`s arguments only for those components.

Now that you point it out, it would make sense to follow shouldComponentUpdate() etc, but it will mean passing this.state into getStyles() many times where it isn't needed (if that matters).

@oliviertassinari
Copy link
Member Author

I think that consistency is more important, I have submitted a new PR to follow the code base 👍.

Now that you point it out, it would make sense to follow shouldComponentUpdate() etc, but it will mean passing this.state into getStyles() many times where it isn't needed (if that matters).

I had this concern too. In our specific case, we will be passing the context while we will not use it.

@mbrookes
Copy link
Member

mbrookes commented Apr 2, 2016

In our specific case, we will be passing the context while we will not use it.

This is also true, but I believe such cases are the minority.

@zannager zannager added the component: autocomplete This is the name of the generic UI component, not the React module! label Mar 14, 2023
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!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Autocomplete] Popover not closing or taking on the correct width
3 participants