feat(vertical-navigation): Add the Vertical Navigation pattern #88

mturley · 2017-11-27T23:11:10Z

What:

Creating the new Vertical Navigation component.

Why:

To bring patternfly-react one step closer to feature parity with patternfly-ng

How:

New components, factoring things to be as reusable as possible

Storybook: https://mturley.github.io/patternfly-react/?selectedKind=Vertical%20Navigation&selectedStory=Items%20as%20JSX&full=0&addons=1&stories=1&panelRight=0&addonPanel=storybooks%2Fstorybook-addon-knobs

mturley · 2017-11-27T23:12:22Z

@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.

priley86 · 2017-11-28T14:51:53Z

@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...

ohadlevy · 2017-11-28T14:53:05Z

/cc @amirfefer

mturley · 2017-12-02T03:20:20Z

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.

jeff-phillips-18 · 2017-12-07T13:21:57Z

src/components/VerticalNavigation/VerticalNavigation.js

+      activeSecondary,
+      children,
+      topBannerContents,
+      notificationDrawer /* TODO notification drawer components? */,


I would leave this out and allow the application to add it separately. The drawer trigger should be added to the topBannerContents and notification drawer would be part of the app content.

Well, that is sort of what I'm doing here already. The notification drawer is a child of the top banner, so it has to be rendered by this component (React doesn't allow external DOM modifications). So we just accept a prop notificationDrawer that is just the entire node tree to be rendered in that container. The application can pass a whole other component as that prop.

I'll remove my TODO though, I was wondering whether we wanted to drive it any more directly here. Sounds like we don't.

I don't believe notificationDrawer needs to be a child of the top banner.

If it is part of the vertical navigation, it does buy you the correct top positioning and height otherwise it is not necessary, the application would then need to specify those values. We could handle that in the notification drawer component.

Just seems better to keep them separated to me. I'm OK either way though.

I see. Okay. It was a child of the top banner in the pf-ng code and the PF core example markup. If we don't do that, I feel like we should make that change upstream maybe?

there is no upstream change necessary though. You can have it either way. There is just the preset position and height setting gained by having the drawer-pf a child of navbar-pf-vertical

jeff-phillips-18 · 2017-12-07T13:23:56Z

src/components/VerticalNavigation/VerticalNavigation.js

+    const { hideTopBanner } = this.state;
+    this.setState({
+      hideTopBanner: !hideTopBanner,
+    });


This will need to notify the application as well. The main body content needs to respond to the collapse as well.

Good point. I'll add a handler that can be passed in for that.

jeff-phillips-18 · 2017-12-07T13:32:04Z

src/components/VerticalNavigation/VerticalNavigationItem.js

+          // This class is confusingly named, but the logic is more readable.
+          'mobile-secondary-item-pf':
+            mobileItem && depth === 'primary' && showMobileTertiary,
+          // I don't know, that's just how this stuff was in patternfly-ng...


Yeah, could have been named better but basically it is used to handle mobile navigation where only one of the menus (primary, secondary, or tertiary) are shown at a time (the navigation is drill down in mobile mode).

Makes sense. I'll change this comment to be a little less confused, haha.

mturley · 2017-12-07T14:21:32Z

src/components/VerticalNavigation/VerticalNavigationItem.js

+                />
+                <span>{title}</span>
+              </div>
+              <ListGroup>


@priley86 , here is the cause of the <li>-inside-<li> warnings we are getting. This ListGroup sits inside a ListGroupItem, and renders ListGroupItem children.

I think you need to add componentClass="ul" here.

priley86 · 2017-12-07T19:07:35Z

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

ERROR in ./src/index.js
Module not found: Error: Can't resolve './components/VerticalNavigation' in

jeff-phillips-18 · 2017-12-07T19:16:28Z

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

jeff-phillips-18 · 2017-12-07T19:32:47Z

src/components/VerticalNavigation/VerticalNavigation.js

+              'show-mobile-nav': showMobileNav,
+            })}
+          >
+            <ListGroup>


I think you need to add componentClass="ul" here.

jeff-phillips-18 · 2017-12-07T19:33:09Z

src/components/VerticalNavigation/VerticalNavigationItem.js

+                />
+                <span>{title}</span>
+              </div>
+              <ListGroup>


I think you need to add componentClass="ul" here.

jeff-phillips-18 · 2017-12-07T20:47:30Z

src/components/VerticalNavigation/VerticalNavigationItem.js

+
+VerticalNavigationItem.defaultProps = {
+  trackActiveState: true,
+  trackHoverState: true,


why are the defaults true here? Aren't these indicating the item is active or hovered on? Only one item at most should ever be active, same as hover.

jeff-phillips-18 · 2017-12-07T20:48:44Z

src/components/VerticalNavigation/VerticalNavigationItem.js

+          [`${nextDepth}-nav-item-pf`]:
+            depth !== 'tertiary' && children && children.length > 0,
+          active: trackActiveState, // This is the only class we have at the tertiary depth.
+          'visible-xs-block': trackActiveState, // TODO is trackActiveState the right thing here, and is it named right?


Not sure why 'visible-xs-block' set on active items (or ever).

jeff-phillips-18 · 2017-12-07T21:01:47Z

src/components/VerticalNavigation/VerticalNavigationItem.js

+      handleTertiaryBlur,
+      handlePrimaryClick,
+      handleSecondaryClick,
+      handleTertiaryClick,


Why would an item have 3 levels of click handlers? Any one item would only ever be at one level right? Shouldn't it just have a handleClick, handleHover, handleBlur?

Not really sure why any item needs the handleHover or handleBlur callbacks. Shouldn't the hover state be maintained here? Also, you will find the need to have delays for the hover/blur handling when showing/hiding submenus.

mturley · 2017-12-10T19:50:36Z

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

jeff-phillips-18 · 2017-12-11T17:48:32Z

@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?

priley86 · 2017-12-12T22:54:40Z

src/components/VerticalNavigation/VerticalNavigation.js

+      collapsedSecondaryNav: false,
+      collapsedTertiaryNav: false,
+    };
+    this.handleNavBarToggleClick = this.handleNavBarToggleClick.bind(this);


We can use the bind helpers in helpers.js here now:
https://github.com/patternfly/patternfly-react/blob/master/src/common/helpers.js

(see Wizard examples)

priley86 · 2017-12-12T22:57:05Z

src/components/VerticalNavigation/VerticalNavigation.stories.js

+  () => {
+    return (
+      <VerticalNavigation secondaryCollapsed={false} tertiaryCollapsed>
+        <VerticalNavigationItem title="Item 1" />


This is a nit but can we expose the API as VerticalNavigation.Item ... see Wizard examples...it's just a consistency thing with the other complex components (both in PF React and React Bootstrap) where we have a lots of sub components.

priley86 · 2017-12-12T22:58:47Z

@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.

priley86 · 2017-12-12T23:10:30Z

@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 <Route> 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...

priley86 · 2017-12-13T01:40:07Z

src/components/VerticalNavigation/VerticalNavigationItem.js

+
+    const handleHover = {
+      primary: handlePrimaryHover,
+      secondary: handleSecondaryHover,


Re-reading this bit... I almost wonder if splitting out Secondary and Tertiary into their own components makes more sense (as it seems it would split the components a bit better as far as props/handlers/etc, i.e.:

<VerticalNavigation > <VerticalNavigation.Item title="Item 1" secondaryCollapsed={false}> <VerticalNavigation.SecondaryItem title="Item 1-A" tertiaryCollapsed> <VerticalNavigation.TertiaryItem title="Item 1-A-i"> </VerticalNavigation.SecondaryItem> <VerticalNavigation.Item> </VerticalNavigation>

It seems like this may split the logic a bit better and make it a bit easier to compose...
@jeff-phillips-18 @mturley thoughts?

I may be over simplifying a bit though... not sure.

mturley · 2017-12-18T18:08:16Z

@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.

mturley · 2018-01-30T22:39:02Z

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!

jeff-phillips-18

LGTM. Thanks for all the hard work that went into this @mturley !

priley86 · 2018-01-31T15:26:46Z

@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

mturley · 2018-01-31T18:25:08Z

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.

priley86 · 2018-01-31T18:42:27Z

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

serenamarie125 · 2018-02-01T00:27:26Z

@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 :)

serenamarie125

@mturley this looks great! Nice to see the different examples as well. I tried a number of different combinations and all looks good 👍

jeff-phillips-18 · 2018-02-01T14:18:33Z

Found a few issues with mobile mode.

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

jeff-phillips-18

Seeing some issues in mobile

mturley · 2018-02-02T19:14:52Z

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

mturley · 2018-02-02T19:15:26Z

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

jeff-phillips-18 · 2018-02-02T21:38:10Z

🎉

mturley · 2018-02-02T21:39:15Z

* feat(deps): apply v4 updates * [cleanup] add back missed storybook type pkgs * [cleanup] make storybook work again

mturley self-assigned this Nov 27, 2017

mturley requested a review from priley86 November 27, 2017 23:11

mturley force-pushed the feature/vertical-navigation branch from f41321b to 07a0358 Compare December 1, 2017 16:47

jgiardino added the in progress label Dec 5, 2017

jgiardino mentioned this pull request Dec 5, 2017

Component - Vertical Navigation #56

Closed

mturley force-pushed the feature/vertical-navigation branch from bee7f02 to 45a5dec Compare December 6, 2017 20:58

jeff-phillips-18 reviewed Dec 7, 2017

View reviewed changes

mturley commented Dec 7, 2017

View reviewed changes

jeff-phillips-18 reviewed Dec 7, 2017

View reviewed changes

mturley force-pushed the feature/vertical-navigation branch 2 times, most recently from d435940 to 2d2467e Compare December 10, 2017 19:49

priley86 reviewed Dec 12, 2017

View reviewed changes

priley86 reviewed Dec 13, 2017

View reviewed changes

mturley force-pushed the feature/vertical-navigation branch 2 times, most recently from 666aa86 to d0bc6c9 Compare December 18, 2017 02:52

jeff-phillips-18 approved these changes Jan 30, 2018

View reviewed changes

mturley mentioned this pull request Jan 31, 2018

Move controlled.js helper into its own package? #200

Closed

priley86 previously approved these changes Jan 31, 2018

View reviewed changes

jeff-phillips-18 added the ux review label Jan 31, 2018

serenamarie125 previously approved these changes Feb 1, 2018

View reviewed changes

serenamarie125 added ux approved and removed ux review labels Feb 1, 2018

jeff-phillips-18 requested changes Feb 1, 2018

View reviewed changes

benjaminapetersen mentioned this pull request Feb 1, 2018

[WIP] Drive vertical nav by array of nav items patternfly/patternfly-react-demo-app#13

Open

mturley dismissed stale reviews from serenamarie125 and priley86 via ae513f9 February 2, 2018 19:12

mturley force-pushed the feature/vertical-navigation branch from 0ce6339 to ae513f9 Compare February 2, 2018 19:12

Add VerticalNavigation, Masthead, Item, and related components

279a64b

mturley force-pushed the feature/vertical-navigation branch from ae513f9 to 279a64b Compare February 2, 2018 19:14

jeff-phillips-18 approved these changes Feb 2, 2018

View reviewed changes

priley86 approved these changes Feb 2, 2018

View reviewed changes

jeff-phillips-18 merged commit d0795c3 into patternfly:master Feb 2, 2018

jgiardino removed the in progress label Feb 2, 2018

mturley deleted the feature/vertical-navigation branch February 2, 2018 21:39

feat(vertical-navigation): Add the Vertical Navigation pattern #88

feat(vertical-navigation): Add the Vertical Navigation pattern #88

Conversation

mturley commented Nov 27, 2017 • edited Loading

mturley commented Nov 27, 2017

priley86 commented Nov 28, 2017

ohadlevy commented Nov 28, 2017

mturley commented Dec 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mturley Dec 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

priley86 commented Dec 7, 2017

jeff-phillips-18 commented Dec 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mturley commented Dec 10, 2017

jeff-phillips-18 commented Dec 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

priley86 Dec 12, 2017 • edited Loading

Choose a reason for hiding this comment

priley86 commented Dec 12, 2017 • edited Loading

priley86 commented Dec 12, 2017 • edited Loading

priley86 Dec 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mturley commented Dec 18, 2017

mturley commented Jan 30, 2018

jeff-phillips-18 left a comment

Choose a reason for hiding this comment

priley86 commented Jan 31, 2018

mturley commented Jan 31, 2018

priley86 commented Jan 31, 2018

serenamarie125 commented Feb 1, 2018

serenamarie125 left a comment

Choose a reason for hiding this comment

jeff-phillips-18 commented Feb 1, 2018

jeff-phillips-18 left a comment

Choose a reason for hiding this comment

mturley commented Feb 2, 2018

mturley commented Feb 2, 2018

jeff-phillips-18 commented Feb 2, 2018

mturley commented Feb 2, 2018

mturley commented Nov 27, 2017 •

edited

Loading

mturley Dec 7, 2017 •

edited

Loading

priley86 Dec 12, 2017 •

edited

Loading

priley86 commented Dec 12, 2017 •

edited

Loading

priley86 commented Dec 12, 2017 •

edited

Loading

priley86 Dec 13, 2017 •

edited

Loading