Bug - Page - navbar regressions due to Toolbar/PageHeaderTools responsiveness

[ianw]

I have recently done an update of our @patternfly/react-core from 4.198.5 -> 4.235.7

This has changed the behaviour of our top PageHeader topNav navbar in a way I can't quite figure out how to work around. Firstly a video of the existing site

Screencast.from.12-09-22.10.18.16.webm

You can see

• The PageHeader has a topNav which remains on a "second" line underneath the existing header at all times. As it gets smaller, it shrinks to have scroll arrows
• The logo just keeps getting smaller as this happens

Below is a sample from the site when updated to 4.235.7

Screencast.from.12-09-22.10.31.45.webm

The problems here

• The logo is missing? As the page shrinks, about the time we get the three-dot kebab for "Components API" etc. it suddenly reappears.
• We don't want the nav bar in the title bar at any point. Eventually it jumps down as the window size shrinks, but we'd like it there always.

From what I can tell, this relates to #6827? I say this because on the "new" site the page div has

<div class="pf-c-page pf-m-resize-observer pf-m-breakpoint-xl pf-m-height-breakpoint-2xl">

whereas it does not have the resize observers on the old site, and I think this change added them.

I guess my main question is how I can keep the topNav separate on all page widths? This may be my misunderstanding of how to use the getBreakpoint function. The secondary question is about why the <PageHeader> logo is disappearing.

The code that is doing this is all around https://opendev.org/zuul/zuul/src/branch/master/web/src/App.jsx#L444

A live sample of this site is available from our CI

https://storage.gra.cloud.ovh.net/v1/AUTH_dcaab5e32b234d56b626f72581e3644c/zuul_opendev_logs_bdb/855577/1/check/zuul-build-dashboard-opendev/bdb7872/npm/html/tenants

Click on "builds" for any tenant and it will move to this view with the top nav bar

[ianw]

I don't know if I'm conflating too many things here -- but this feels similar. I tried updating this to use a Masthead with embedded Toolbar (which seems to be what is recommended over page headers now?).

This seems to be something like what the sample page/s do (e.g. https://www.patternfly.org/v4/components/page/react-demos/sticky-section-group/)

However, in what looks like similar behaviour -- I can not get the gear icon/tz selector to float right and stay right. See below how this looks correctly floated to the right when very narrow, but as soon as it seems to recalculate its width it seems to loose the width of the MastheadContent and everything goes left again.

Screencast.from.12-09-22.14.08.18.webm

The mashead puts these in a toolbargroup with alignment alignRight

      <Masthead>
        <MastheadToggle>
          <Button variant="plain"><BarsIcon />
          </Button>
        </MastheadToggle>
        <MastheadMain>
          <MastheadBrand>
            <Brand src={logo} alt='Zuul logo' />
          </MastheadBrand>
          <MastheadContent>
            <Toolbar isFullHeight isStatic>
              <ToolbarContent>
                <ToolbarGroup
                  alignment={{ default: 'alignRight' }}
                  spacer={{ default: 'spacerNone', md: 'spacerMd' }}>
                  <ToolbarItem>
                    <SelectTz />
                  </ToolbarItem>
                  <ToolbarItem>
                    <ConfigModal />
                  </ToolbarItem>
                </ToolbarGroup>
              </ToolbarContent>
            </Toolbar>
          </MastheadContent>
        </MastheadMain>
      </Masthead>

This is then simply included as the Page header

      <React.Fragment>
        <Page header={header}>page content</Page>
      </React.Fragment>

[jschuler]

Hi @ianw , that's unfortunate that a react-core upgrade is breaking your UI. In general the PatternFly team tries to ensure that no breaking changes get introduced, but looks like a change slipped through the cracks.

EDIT: Disregard the changes below, see next comment

I had a look at your project. With regards to the disappearing svg image: It appears that the custom style for .zuul-brand gets overwritten by the PF .pf-c-brand. I would suggest to increase the specificity for it .pf-c-brand.zuul-brand {}

For the toolbar layout changes, it would probably be easiest to ditch the PageHeader topNav prop, and instead use the Page tertiaryNav prop to add your {nav} to. With a few custom style overrides you can make it look the same as before.

Here are the changes

--- a/web/src/App.jsx
+++ b/web/src/App.jsx
@@ -447,7 +447,6 @@ class App extends React.Component {
         logoProps={{ to: logoUrl }}
         logoComponent={Link}
         headerTools={pageHeaderTools}
-        topNav={nav}
       />
     )
 
@@ -455,7 +454,7 @@ class App extends React.Component {
       <React.Fragment>
         {notifications.length > 0 && this.renderNotifications(notifications)}
         {this.renderConfigErrors(configErrors)}
-        <Page header={pageHeader}>
+        <Page className="zuul-page" header={pageHeader} tertiaryNav={nav}>
           <ErrorBoundary>
             {this.renderContent()}
           </ErrorBoundary>
diff --git a/web/src/index.css b/web/src/index.css
index e47cfc63c..1c356cb47 100644
--- a/web/src/index.css
+++ b/web/src/index.css
@@ -24,12 +24,21 @@ a.refresh {
 }
 
 /* Navigation bar */
-.zuul-brand {
+.pf-c-brand.zuul-brand {
     /* TODO (felix): If we can define a size in the logo.svg itself, this
        shouldn't be necessary. */
     height: 40px;
 }
 
+.zuul-page .pf-c-page__header {
+    min-height: 0;
+}
+
+.zuul-page .pf-c-page__main-nav {
+    background-color: var(--pf-global--BackgroundColor--dark-300);
+    padding-top: 0;
+}
+
 .zuul-select-tz {
     /* That's the color PF4 uses for the dropdown items in the navbar */
     color: var(--pf-global--Color--dark-100);
@@ -429,27 +438,6 @@ details.foldable[open] summary::before {
     content: "less";
 }
 
-/* Overwrite PF4 navbar rules to enforce the 'lg' layout of the navbar also for
-   'xl' sized screens. This will keep the navbar stacked (like it was in Zuul
-   before) and prevent PF4 to put all elements in a single line with horizontal
-   scrolling (which would look something like:
-   ZUUL | < Status Projects Jobs Labels No... > | API Documentation Tenant */
-@media screen and (min-width: 1200px) {
-    .pf-c-page__header-nav {
-        grid-column: 1/-1;
-        grid-row: 2/3;
-    }
-
-    .pf-c-page {
-        --pf-c-page__header-nav--PaddingRight: var(
-            --pf-c-page__header-nav--lg--PaddingRight
-        );
-        --pf-c-page__header-nav--PaddingLeft: var(
-            --pf-c-page__header-nav--lg--PaddingLeft
-        );
-    }
-}
-

It would probably be possible to also rework it using the MastHead components, but I have not tried that route yet.

[jschuler]

So looks like there was a change made in react-styles a while back that added .mjs files. This is what they look like:
https://unpkg.com/browse/@patternfly/react-styles@4.52.22/css/components/Page/page.mjs

It appears it is loading the CSS stylesheets asynchronously since it is using the import function (i.e. import('./page.css');).

It seems these should be change to use the regular import statement, i.e. import './page.css' instead.

[ianw]

It appears it is loading the CSS stylesheets asynchronously since it is using the import function (i.e. import('./page.css');).

Oh interesting. This might explain a lot of weirdness I've seen when I looked at this logo closer -- I found that specifying the width/height in the SVG (currently it only has a viewport) made it work. I couldn't pin down why it would sometimes show and sometimes not, and differ depending on if it's in our CI system or my local development. CSS applying asynchronously or differently in some situations could explain that!

[ianw]

@jschuler just to clarify -- and this is mostly my fault because I've overloaded this issue with a couple of things -- is the CSS import only affecting the logo coming and going, or do you think that's also overriding the navbar bits and making that jump up as well? i.e. should we still look at moving it into the tertiary nav irrespective of page.css imports?

... update ...

These two changes look pretty good as simplification updates to me independent of any bugs around loading css and I proposed them to upstream with

• https://review.opendev.org/c/zuul/zuul/+/857244
• https://review.opendev.org/c/zuul/zuul/+/857245

Thanks!

[jschuler]

I agree, I think those code changes are fine regardless of the CSS load ordering issue
Thanks to @mcoker who identified the stylesheets loading in a different order, it is possible that you might have other styles overwritten. Basically with this bug assume that your custom styles are loaded first, and unless they're more specific than the PF styes they're targeting, could lose out.
We will put up a bug fix for the load ordering issue in the meantime though, so a newer release should not have that issue anymore.

[jschuler]

@ianw Fix is included with the prerelease version @patternfly/react-core@4.237.10. You can upgrade to that or wait until the official released version over the next few days. Thanks for reporting the issue!