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

Side nav - [new component] #1319

Closed
wants to merge 10 commits into from
Closed

Side nav - [new component] #1319

wants to merge 10 commits into from

Conversation

yongxu
Copy link
Contributor

@yongxu yongxu commented Aug 2, 2015

Hi there,
I started this component for my own project use, but I think it would be great to contribute it as a part of material ui library.
A better LeftNav with much richer features, Items are defined react components instead of list.
Also compatible with ListItem, used Menus/Menu instead of Menu/Menu.

The code is structurally similar to LeftNav, which was intended to create a better version of LeftNav. Some function names and the swipe handling functions come from LeftNav.
I also added a demo page, it works great, but more detailed documentation is still needed.

Demo screen shot:
screen shot
screen shot

This was referenced Aug 2, 2015
@yongxu yongxu changed the title Side nav Side nav - [new component] Aug 2, 2015
} = this.props;

let mergedStyles = this.mergeAndPrefix({
/*TODO*/
Copy link
Contributor

Choose a reason for hiding this comment

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

So this component basically should just add some styles to MenuDivider, but for now it does'n do even that. Do you think this component is really needed?

@igorbt
Copy link
Contributor

igorbt commented Aug 3, 2015

I expected to see here really a new component. SideNav also means that it can be opened from the right side (just as Material Design specs says).

Instead I saw a copied LeftNav with a different implementation of how children are passed and rendered (one that actually adds a lot of overhead because it uses a heavy Menu component for this).

I think the current LeftNav can be better implemented, by letting a greater flexibility in what you can put there via children (to act as a container actually). I think somebody already proposed this or even works to make this happen.

While this SideNav component may be good for you application and it provides some flexibility, I wouldn't want to see this implementation into framework.

@yongxu
Copy link
Contributor Author

yongxu commented Aug 4, 2015

@igorbt, Thanks for all the feedback!

I think the current LeftNav can be better implemented, by letting a greater flexibility in what you can put there via children (to act as a container actually).

Totally agree with you on this. This is actually the main reason for me to make SideNav.
I made this component in order to replace LeftNav, not inventing something new. I did see a few proposal around (#413,#27) but didn't find any further progress, so I wrote one myself based on LeftNav.

I don't think my work is prefect, but I do believe composable SideNav is better than LeftNav and has penitential to replace it.

@igorbt
Copy link
Contributor

igorbt commented Aug 4, 2015

@yongxu, thanks for clarifications. It make more sense for me now and I think that the direction is good.

Still, it feels to me like a a workaround implementation that can live in a project, but not in a framework. What I would like to see is to extract from Menu a kind of EmbedableMenu (that would do the children rendering and keyboard navigation logic) and let in Menu only the layer opening logic. Then, it would be OK to use this EmbedableMenu inside the SideNav component. And SideNav should support opening from right (woudn't be so hard to do I think, just by abstracting current implementation).

I also should use LeftNav/SideNav in my project, this is why I really took a look at your PR. I'm not sure if I really need a so flexible LeftNav there, but if I do, then I may try to implement what I said above.

It would be great to hear an opinion from a core contributor here.

@hai-cea
Copy link
Member

hai-cea commented Aug 4, 2015

Thanks @yongxu & @igorbt - We're planning on creating a NavDrawer component very soon. It will essentially be a component that takes care of show/hiding and allow the user to put anything in there.

We're definitely trying to simplify things and this PR is going in the right direction in terms of composability. I'm just not sure if we should introduce a component like this when the NavDrawer soon to follow.

@dottgonzo
Copy link

i'm using material-ui without react-router, and need to configure the leftnav. Hope to find NavDrawer or this solution soon :)

@fabiozaffani
Copy link

@hai-cea this component code look's very solid overall, wouldn't be a good idea to use it as the base for the NavDrawer component instead? It also would be a very good substitute for the current LeftNav component which was one of the first ones created in this repository and as such is quite limited (no composability) when compared to the other newer ones.

@yongxu
Copy link
Contributor Author

yongxu commented Aug 21, 2015

Thanks @fabiozaffani!
@hai-cea I have been using this component in my project for about two week, it works quite well and I have never had any problem with it. There are things that I can still improve and I'd love to contribute if there are any features/changes you guys have planned to include in the future NavDrawer.

@mping
Copy link

mping commented Sep 11, 2015

@hai-cea any news on NavDrawer component? Couldn't find anything in the issues

@oliviertassinari oliviertassinari 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 Oct 26, 2015
@alitaheri
Copy link
Member

All the features introduced here are also available in the current LeftNav component. thanks for contributing your code to mui. But i guess it's not needed any more. @oliviertassinari Do you think we should close this?

@oliviertassinari
Copy link
Member

That's far from the current master branch.
We can now pass a List component to the LeftNav. I believe that you are right, features introducted are now available.

I'm closing this PR. However, I really like the example and I think that it's valuable. @yongxu Thanks. Feel free to open a new PR to improve our LeftNav examples :).

mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Nov 10, 2020
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.

8 participants