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

Menu has nested items support using popover #2148

Merged
merged 2 commits into from
Nov 26, 2015

Conversation

chrismcv
Copy link
Contributor

Reworked from #1845.

This one might need some UX review.

anchorEl={this.state.anchorEl}
open={this.state.open}
onRequestClose={this._onRequestClose}>
<Menu desktop={desktop} disabled={disabled} style={{position:'relative'}}>
Copy link
Member

Choose a reason for hiding this comment

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

I think that we should move {position:'relative'} in a shared variable.
I think that we should add PureRenderMixin to the Menu component in the future.
This would prevent unnecessary rerender.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@oliviertassinari
Copy link
Member

@chrismcv I have just made minor feedbacks. Nice work 👍.
Edit: I have found a bug, on the example of the documentation, closing the second level of menu don't close the third level.

@oliviertassinari oliviertassinari 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 Review: review-needed labels Nov 24, 2015
@chrismcv
Copy link
Contributor Author

@oliviertassinari - bug fixed, and other changes made. Can squash if you like.

@chrismcv
Copy link
Contributor Author

@oliviertassinari - want a squash here?

@oliviertassinari
Copy link
Member

Let's take care of this issue first

Edit: I have found a bug, on the example of the documentation, closing the second level of menu don't close the third level.

I this something easy to fix?

@chrismcv
Copy link
Contributor Author

@oliviertassinari - how about now? I think we should hold off merging until it works. (I'm around today and tomorrow to resolve further if needs be)

@oliviertassinari
Copy link
Member

It's working now 👍.
I have one last issue 😁
nov 26 2015 21 42

@chrismcv
Copy link
Contributor Author

@oliviertassinari should be good now! (squashed too)

@oliviertassinari
Copy link
Member

Need a rebase and 🔥
nov 26 2015 22 49

@chrismcv
Copy link
Contributor Author

😭 sorted I think

@chrismcv
Copy link
Contributor Author

@oliviertassinari - done

@oliviertassinari
Copy link
Member

@chrismcv Finally, I still see some weird stuff, but I think that it should be ok for a first iteration!

oliviertassinari added a commit that referenced this pull request Nov 26, 2015
[Menu] Add nested items support using popover
@oliviertassinari oliviertassinari merged commit f300340 into mui:master Nov 26, 2015
@chrismcv
Copy link
Contributor Author

@oliviertassinari yeah - apologies that took so long! thanks 🎉

@oliviertassinari
Copy link
Member

@chrismcv I can start working on enforcing new eslint rules 😉

@chrismcv chrismcv deleted the menu-nested branch November 26, 2015 22:32
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.

2 participants