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

[IconMenu] Set container as anchorEl when using prop 'open' #3666

Merged
merged 2 commits into from
Mar 12, 2016

Conversation

crashbell
Copy link
Contributor

  • PR has tests / docs demo, and is linted.
  • Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
  • Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

IconMenu allows to use prop open to open the popover but the anchorEl isn't set at the first load so this causes a wrong position.

iconmenu-prop-open-error

if (nextProps.open !== null) {
this.setState({
open: nextProps.open,
anchorEl: ReactDOM.findDOMNode(this.refs.iconMenuContainer),
Copy link
Member

Choose a reason for hiding this comment

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

findDOMNode is not needed as the target is a dom element. Ideally, it would be even better if you could use callback refs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @alitaheri!, I removed findDOMNode.

@alitaheri alitaheri added 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 Mar 11, 2016
@alitaheri
Copy link
Member

Thanks for finding this bug 👍 👍

@alitaheri
Copy link
Member

Thanks a lot 👍

@callemall/material-ui Looking good 😁

@alitaheri alitaheri added PR: Review Accepted and removed PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Mar 12, 2016
@mbrookes
Copy link
Member

Looks good here too, but I notice that the 'Controlled examples' aren't truly controlled - only the selected value(s) for which there's no internal state anyway.

@alitaheri - would it make sense to update the controlled examples so the open state is controlled, perhaps with one example open by default?

@alitaheri
Copy link
Member

@mbrookes Hmmm, I guess that's why we didn't notice this bug in the first place O.o Good catch 👍 @crashbell Would that be ok to add an example for this to prevent future regressions? that would be appreciated, thanks a lot 👍 👍 :grin

@mbrookes mbrookes added 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 PR: Review Accepted labels Mar 12, 2016
@crashbell
Copy link
Contributor Author

Let me try to update the docs. Thanks for reviewing this PR.

@mbrookes mbrookes added the bug 🐛 Something doesn't work label Mar 12, 2016
@crashbell
Copy link
Contributor Author

While making the sample I found out that this issue was handled in this code but I don't know why findDOMNode doesn't work and it returns the null el in my project so I got the error

Uncaught TypeError: Cannot read property 'getBoundingClientRect' of null

It's working properly with the docs code.
I'm going to close this and dig into my project setup to figure out this.
Thanks all!

@crashbell crashbell closed this Mar 12, 2016
@crashbell crashbell deleted the add_anchor_el branch March 12, 2016 19:36
@crashbell
Copy link
Contributor Author

I found out the root cause here: #3647
We haven't released this PR yet, therefore, I will wait for the release so this bug will be gone.

@crashbell crashbell restored the add_anchor_el branch March 12, 2016 19:49
@alitaheri
Copy link
Member

@crashbell Thanks for getting to the bottom of it 👍 We'll release another alpha soon 😁

@crashbell
Copy link
Contributor Author

It's quite annoying when I open/close this PR few times but I found out the fix is still valid. You can compare my photos below, the behavior of popover is a bit different without my fix.

Without IconMenu fix
screen shot 2016-03-13 at 2 59 12 am

With IconMenu fix
screen shot 2016-03-13 at 2 58 45 am

I also add an example for this:
screen shot 2016-03-13 at 3 12 55 am

@alitaheri
Copy link
Member

This is looking really good, thanks a lot for tackling this 👍

@callemall/material-ui Take a look.

@alitaheri alitaheri added PR: Review Accepted and removed PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Mar 12, 2016
oliviertassinari added a commit that referenced this pull request Mar 12, 2016
[IconMenu] Set container as anchorEl when using prop 'open'
@oliviertassinari oliviertassinari merged commit 7aeadea into mui:master Mar 12, 2016
@oliviertassinari
Copy link
Member

@crashbell Probably this week, have a look at #3700.

@mbrookes
Copy link
Member

@crashbell In any case you don't need to monkey patch - just use HEAD@3c77dba97. 👍

@crashbell
Copy link
Contributor Author

thanks @oliviertassinari and @mbrookes so much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants