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

LeftNav is still using old menu -- which will be deprecated soon #2491

Closed
sothychan opened this issue Dec 12, 2015 · 5 comments
Closed

LeftNav is still using old menu -- which will be deprecated soon #2491

sothychan opened this issue Dec 12, 2015 · 5 comments
Labels
bug 🐛 Something doesn't work

Comments

@sothychan
Copy link

Is this on purpose? Nevertheless, there's a serious bug with left nav where if you were to pass in the menu-item component from the new menu, the code in left-nav completely bypasses the logic to attach the event handler to it so clicking on the menu items do not fire any events.

I am strongly hoping that this is an oversight. Regardless, I've already forked the project and plan to fix the issue. I will submit a PR when it's ready.

@alitaheri
Copy link
Member

@sothychan menuItems will be deprecated too, see #2456. I wouldn't bother If I were you 😁

@alitaheri
Copy link
Member

And within the PR deprecating that, we'll address all the issues regarding this. Also, there is debate whether the LeftNav should actually ever attack a handler. Making assumptions is bad for production. So maybe try controlling LeftNav via it's open prop and onRequestChange, and add a handler to your menu-items handling the closing. This library tries to cover too many use-cases, where as react's philosophy is composablity. I believe this library should do one thing very good: implement Material Design with good customization. But it makes too many assumptions. These issue must be addressed!

@alitaheri alitaheri added the bug 🐛 Something doesn't work label Dec 12, 2015
@alitaheri alitaheri added this to the 0.14.0 Release milestone Dec 12, 2015
@sothychan
Copy link
Author

Excellent. As a workaround, I just added onTouchTap events to my menuItems for now. Glad to hear that it's on the radar. I will take your advice and not bother with it :).

I am interested and can make myself available to help if you guys need. If so, please PM me and give me the details on how you guys envisioned how it should be built.

@sothychan
Copy link
Author

@subjectix When you say that menuItems will be deprecated, you're talking about the old one, correct? The one here: /material-ui/src/menu/*? The one in /material-ui/src/menus are still valid, correct? Or, are you getting rid of menu items altogether and use list items instead since it seems like it just renders that underneath the hood?

@alitaheri
Copy link
Member

@sothychan I'm talking about using menuItems prop. i.e. using JSON to represent the children of the LeftNav for the sake of composability. This way, the surface API will be much cleaner and much easier to customize than having one single component doing so many irrelevant things. the old ones will be removed and the new ones will always be used in a composable way. @sothychan getting rid of the menus might not be that easy though. but makikng them composable just like ListItem is the first step :D

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

No branches or pull requests

2 participants