add custom masthead

[priley86]

Getting closer here...

Still a couple problems:

• expand / collapse seems to work on desktop, but when clicking the hamburger on mobile, I do not see expand working...

• With the navbar collapsed, it seems like we are missing some classes and the icons are not updating after page route switches.
  

@jeff-phillips-18 any ideas?

[aljesusg]

In verticalNav component Masterhead is passed like prop because should be inside the nav component, maybe you are missing classes by this

[priley86]

@aljesusg either way should work (using the masthead prop on VertNav... or just creating a separate JSX). I just tested and found that doing:

      <React.Fragment>
        <VerticalNav
          persistentSecondary={false}
          masthead={
            <Masthead
              iconImg={pfLogo}
              titleImg={pfBrand}
              title="PatternFly React Demo App"
              onNavToggleClick={() => this.onCollapse()}
            >
...

exhibits the same behavior...

[aljesusg]

My point was about classes in the patternfly documentation masthead is inside the nav generated by verticalNav, so maybe there is some class missing, about the version mobile maybe is a bug

[jeff-phillips-18]

@priley86 Setting the navCollapsed does not handle mobile hide/show for the nav menus. Looks like you need to set the controlledState mobile and showMobileNav props.

[priley86]

@mturley - i think you are probably most familiar. Any ideas?

[mturley]

@priley86 I am just now coming back around to this mention. I may have some time later this week to look at this. @jeff-phillips-18 is correct that navCollapsed is only for the desktop collapse/expand of the nav, and not the mobile and showMobileNav props. This is an artifact of me mostly 1:1 copying and refactoring the logic from the Angular implementation. Perhaps they should be the same prop, but that'd be a breaking change.

[jeff-phillips-18]

@mturley Yes, I believe the navCollapsed is only for desktop mode.

[mturley]

I can look at fixing this myself tomorrow, but it seems to be a case of badly named props that I ripped off from pf-ng variable names while I ported over the logic. Not great.

[mturley]

the problem is that the hamburger menu should do something differently in mobile mode and desktop mode, with respect to the props you need to pass in stateless (controlled) mode. The stateful behavior built in should be a good guideline for how that works, i'll see if i can find a good snippet.

[mturley]

Okay, @priley86 @aljesusg this is the method inside VerticalNav that is used when you click the hamburger menu in the default masthead. Overriding it causes this to no longer be run, which is perhaps a bug in the stateful mode of the nav. But to use it statelessly, we can just replicate this behavior out in the demo app:

https://github.com/patternfly/patternfly-react/blob/620ffea6e11808b90ce7cce3adbc782c6635ba6d/packages/patternfly-react/src/components/VerticalNav/VerticalNav.js#L264

updateNavOnMenuToggleClick = () => {
    const {
      onMenuToggleClick,
      isMobile,
      showMobileNav,
      navCollapsed,
      setControlledState
    } = this.props;
    if (isMobile) {
      if (showMobileNav) {
        setControlledState({ showMobileNav: false });
      } else {
        this.setMobilePath(null);
        setControlledState({ showMobileNav: true });
      }
    } else if (navCollapsed) {
      this.expandMenu();
    } else {
      this.collapseMenu();
    }
    onMenuToggleClick && onMenuToggleClick();
};

collapseMenu = () => {
    const { onCollapse, setControlledState } = this.props;
    setControlledState({ navCollapsed: true });
    onCollapse && onCollapse();
  };

  expandMenu = () => {
    const { onExpand, setControlledState } = this.props;
    setControlledState({ navCollapsed: false });
    onExpand && onExpand();
};

So in short, when in mobile screen sizes, the mobile prop should be true, and the button needs to toggle showMobileNav. In desktop screen sizes, the mobile prop should be false, and the button needs to toggle navCollapsed. These should maybe just be the same prop, in a future breaking change. And passing a custom masthead totally breaks the stateful/default behavior. So that's two bugs, I suppose, but I'm not sure how we would fix the latter.

[mturley]

As for the active page not being set properly, we need to set the activePath prop, which is a string matching the idPath of a nav item. This is just the id of each item in the path from primary to secondary, delimited by slashes, i believe... it's been a while.

[priley86]

hey @mturley - i appreciate this. It's helpful to understand the inner workings here.

That being said, it seems like this might not be as easy to use out of the box as I thought (particularly with custom masthead). Since custom masthead is still important in many spots, I could see this being important for some consumers. Do you mind following up in an issue w/ PF React (or maybe an existing issue if one already exists)? Since this is PF3, it is obviously low priority, but I think it may help to have an issue so we know to look out for this in the future.

I will just close the PR for now if that's OK and we can reference it later...

[mturley]

Ok, @priley86 we can do that. Should we also fix this PR so that the masthead works in the demo app in the meantime? I'll open an issue for this today on pf-react and reference this PR either way.

[mturley]

Not sure when I can get to this, but I created this issue to track it in pf-react: patternfly/patternfly-react#525

I plan to fix this PR soon by lifting all the state out and working around the bug in the stateful behavior of VerticalNav.

[mturley]

I have a partial fix for this, but still having some trouble with it... My apologies @priley86 ! I'm headed on PTO for my wedding... this will have to wait. I might have a chance to look at it over the weekend. Most likely this will be fixed in early September :(

[mturley]

@priley86 if you or someone else has time and can reproduce these issues in Storybook in patternfly-react, maybe we can get the upstream bugs fixed that way.

[mturley]

So I had a free hour and I started debugging this.. The mobile prop doesn't need to be set, that's already being done internally by VerticalNav, but showMobileNav does need to be toggled instead of navCollapsed when in mobile screen sizes. We can use the layout helper in patternfly-react for the demo app to know whether it's in mobile mode. I opened a PR to export that: patternfly/patternfly-react#539

I'm more worried about the active state not updating. Changing the active prop on an item should be enough. This appears to be an existing problem even in master on the demo app. Actually, it appears there is a bug in the demo app, not in the VerticalNav here. In the render method of App.js:

    const activeItem = this.menu.find(
      item => location.pathname.indexOf(item.to) > -1
    );

I added a console.log(activeItem) below this, and it logs "Ipsum" no matter which nav item I click. So the active prop isn't getting set properly, I think location.pathname is not a reliable way of checking the current route on render maybe?

I should stop working on PTO. @priley86 , hope this helps.

[priley86]

thanks! I pushed a simple fix in #43 . Please feel free to keep iterating on this as you have time!

[mturley]

@priley86 I pushed a fix for the feedback on #43 in #45 :) and thanks for merging patternfly/patternfly-react#539. I'm back from PTO and I'll try to finish fixing this before I get too wrapped up in V2V again.

[mturley]

@priley86 here is a fix for the remaining problem with the expand/collapse on mobile: mturley@53d87c9

If we merge #45, rebase this branch on master, and cherry-pick that commit, I think that should resolve all the issues here.

[priley86]

thanks! will give this a go soon @mturley