@ohadlevy, Serena told me that I should ping you when I had this pushed up. It's not quite done yet, i'm still in the process of refactoring things. I pushed my partial code so I can head home, but I plan to wrap up most of this tonight.

@mturley It looks like it's still a bit early, but I think this is the right idea. I.m.o. the only component state we'll need is the state which indicates whether the secondary/tertiary nav drawers are open, everything else can be indicated by props from the application/redux store (including what nav items are active). I like the approach w/ rendering the nav items as lists.

Look forward to reviewing it further once you are ready here...

/cc @amirfefer

Sorry for the delay on this, I've had a few distractions. I just pushed a few more commits that complete the basic structure of this and include some refactoring I did to make the primary/secondary/tertiary levels all driven by one VerticalNavigationItem (the markup was similar enough at each level that the differences are logically laid out in the render function without foo much fuss). I think it's pretty clean.

Now I just need to finish getting any react-specific CSS tuned up, and address my last few TODO comments. Should be good to go on Monday.

My next PR will be at a faster pace-- I've had some new-hire tasks and other one-time distractions these first few weeks.

@mturley do you mind rebasing w/ current master? I think something in index.js is misaligned. Getting the following:

@mturley Looks like we need index.js in components/VerticalNavigation

I'm almost done with this, I squashed all the commits so far and I'll squash again after I push the rest.

@mturley Would it make sense to create this component w/o secondary or tertiary navigation support to begin with? This gets us partially there (and enough of the way for those w/o secondary menus) and able to try this out before adding the hover/pinned features.

Thoughts?

@mturley apologies - ignore my last comment about mockNavitems - I misread the code ;). I think it's nice providing both API surfaces like you have done.

@mturley once you get closer on this, you may want to test w/ React Router. I don't know that we should necessarily assume that VerticalNavigationItem will be a direct child of VerticalNavigation (depending on how we set up the React Router Dom). Reason being is - typically you will see as a wrapper to any routable component which accepts a match property (as a function). The match property typically determines active state (based on the active route).

I started some examples of this here prior, but this is missing all of the other vert. nav features you've started in the implementation here. I would say just test this sooner than later ;->

https://github.com/patternfly/patternfly-react-demo-app/blob/flowjs/src/components/Nav/VerticalNav.js

https://github.com/patternfly/patternfly-react-demo-app/blob/flowjs/src/components/Nav/RouteNavItem.js

The React Router Dom is of course optional too... you can simply inject the current route state in a parent level wrapper (this might be easier). This would enable you to still use VerticalNavigationItems as direct children...
https://github.com/ReactTraining/react-router/blob/v3/docs/Introduction.md#getting-url-parameters

If the downstream product is not using React Router - then this is likely not an issue though...

@jeff-phillips-18 , sorry I didn't notice your comment earlier about doing primary-only. I think I will have everything working shortly, but if there are any further issues with secondary/tertiary stuff I will disable them for now and get this merged.

Thanks @priley86. I pushed a few more minor tweaks from discussions with @jeff-phillips-18 (fixed corner cases around hover/blur timers). I guess that invalidated your approval. If you could re-approve when you get a chance that'd be great!

@mturley nice job on the fixes. Again I am OK to merge here. Another thing I noted with the controlled pattern here - we may be able to connect that to a redux-session or a redux local storage as an enhancement in the future. This works great for now though!

https://egghead.io/lessons/javascript-redux-persisting-the-state-to-the-local-storage

Thanks for the reviews guys. @priley86 I like the idea of using redux patterns for local storage, I do think though that even if we add that support, it should still be able to fall back on window.sessionStorage to support users who aren't using redux.

@mturley sure - i think redux is an optional replacement for the consumer. valid point

@jeff-phillips-18 @mturley just want to confirm that the strategy for this component was to match the core implementation, which can be found here: https://rawgit.com/patternfly/patternfly/master-dist/dist/tests/vertical-navigation-with-tertiary-pins.html

There is a difference between the Pinning implementation between core & angular. This PR matches the core implementation. Just noting here :)

Found a few issues with mobile mode.

1. if I collapse (only icons) then go into mobile mode, the primary menu should show both icons and text
2. if I collapse (only icons) then go into mobile mode, the tertiary menu gets displayed incorrectly
   
3. In mobile mode, the body content should have no left margin.

Fixed the mobile issues @jeff-phillips-18 was seeing.

Aww every time I rebase on upstream master it dismisses all my shiny green approvals... @priley86 @serenamarie125 :)