chore(DashboardHeader): align DashboardHeader with core #7518

jenny-s51 · 2022-06-06T21:03:59Z

What: Closes #7429

patternfly-build · 2022-06-06T21:13:35Z

Preview: https://patternfly-react-pr-7518.surge.sh

A11y report: https://patternfly-react-pr-7518-a11y.surge.sh

nicolethoen

Looks like the left/right padding in the masthead, as well as the image size of the PatternFly logo are both a little larger on the react dashboard wrapper than the core demos.

Also, should this update include updates to the masthead in the page demos? Those are the only full page demos, i believe, that don't use the dashboard wrapper.

jenny-s51

Thank you for your feedback @nicolethoen ! I think that gif is showing the difference between the react workspaces, i.e. on main vs on this branch. Did you mean to show the difference between the react and core workspaces?

I've increased the size of the masthead inset padding to match the size of the page inset as shown in the core demo, which might be causing the toolbar item spacing to look different by a few pixels. I've also increased the Brand/logo size to match the exact size in the core demos too.

I checked all the HTML elements and the components/styling all do seem to match core. Looks like the toolbar items are still a few pixels away from their placement in core though... would be awesome to get some opinions/feedback on this styling from the core folks! @mattnolting @mcoker

jenny-s51

Also, yes, it looks like we do need to update Page with the new DashboardWrapper @nicolethoen ! I've opened and assigned myself to #7526! Will plan to update those full screen demos with these changes once they are approved 👍

nicolethoen · 2022-06-08T13:18:02Z

Did you mean to show the difference between the react and core workspaces?

oh - you're right. I thought I had the core demo up... my bad.

nicolethoen

It's looking great! The only other thing I'm noticing is that there is a missing kebab menu in the masthead at smaller screen widths

your PR:

core demo:

mcoker · 2022-06-15T18:47:32Z

packages/react-core/src/demos/examples/DashboardHeader.js


    const masthead = (
-      <Masthead>
+      <Masthead inset={{ default: 'insetLg' }}>


The inset should change between mobile/desktop layouts - 16px (insetMd) on mobile (< 1200px or $pf-global--breakpoint--xl by default) and 24px on desktop (insetLg).

We manage that for the user here, if the page isn't set to use the resize observer to manage the breakpoint for mobile/desktop - https://github.com/patternfly/patternfly/blob/53539639ef305b90fcfe9576b6962d8c1a5247be/src/patternfly/components/Masthead/masthead.scss#L160-L162

That seems to imply that if using the resize observer, the user is expected to manage the masthead inset themselves? Or maybe that's an oversight and our resize observer code should apply the appropriate inset manually?

I think that CSS is a bit problematic though, because it isn't just the masthead that has a dynamic inset between mobile/desktop layout. The nav, page sections, table, data list, etc all have a dynamic inset managed by CSS at the mobile/desktop breakpoint. So unless the resize observer is changing all of those things, I think this is going to lead to inconsistent insets.

IMO we could just leave the insets as they are in the masthead to change at our default breakpoint (1200px/$pf-global--breakpoint--xl), which ties the insets to viewport size instead of the configurable layout change that can be set using the resize observer. Seems acceptable to me, and likely the easiest approach. FWIW with the upcoming major version release, design is on board with just using a single inset for all screen sizes, which would alleviate all of this.

@mattnolting you have done the resize observer work so far - wdyt?

@mcoker this seems like an oversight. I agree that page insets should remain based on viewport width, as all other component insets are based on viewport width. Using a consistent page inset resolves these issues, which will be great to have when we can make the update, but until then, leaving the insets as they are and opening a Core issue to update the supporting CSS makes sense to me.

mcoker · 2022-06-15T18:50:17Z

packages/react-core/src/demos/examples/DashboardHeader.js

+              alt="Fallback patternFly default logo"
+            >
+              <source media="(min-width: 768px)" srcSet="/assets/images/logo__pf--reverse-on-md.svg" />
+              <source srcSet="/assets/images/logo__pf--reverse--base.svg" />


mcoker · 2022-06-15T19:57:00Z

packages/react-core/src/demos/examples/DashboardHeader.js

              </ToolbarItem>
            </ToolbarGroup>
-            <ToolbarItem visibility={{ default: 'hidden', sm: 'visible', md: 'visible', lg: 'hidden' }}>
+            <ToolbarItem visibility={{ default: 'visible', sm: 'visible', md: 'visible', lg: 'hidden', xl: 'hidden' }}>


Untested, but I believe all but lg are needed - it should be visible by default, and once it's set at a breakpoint, it persists through subsequent breakpoints by default.

Suggested change

<ToolbarItem visibility={{ default: 'visible', sm: 'visible', md: 'visible', lg: 'hidden', xl: 'hidden' }}>

<ToolbarItem visibility={{ lg: 'hidden' }}>

Great catch @mcoker!! Fixed 👍

mcoker

LGTM!!!

kmcfaul

lgtm!

patternfly-build · 2022-06-22T15:51:33Z

Your changes have been released in:

eslint-plugin-patternfly-react@4.61.17
@patternfly/react-catalog-view-extension@4.73.17
@patternfly/react-charts@6.75.17
@patternfly/react-code-editor@4.63.17
@patternfly/react-console@4.73.17
@patternfly/react-core@4.222.17
@patternfly/react-docs@5.83.17
@patternfly/react-icons@4.73.17
@patternfly/react-inline-edit-extension@4.67.17
demo-app-ts@4.182.17
@patternfly/react-integration@4.184.17
@patternfly/react-log-viewer@4.67.17
@patternfly/react-styles@4.72.17
@patternfly/react-table@4.91.17
@patternfly/react-tokens@4.74.17
@patternfly/react-topology@4.69.17
@patternfly/react-virtualized-extension@4.69.17
transformer-cjs-imports@4.60.17

Thanks for your contribution! 🎉

) * align dashboardwrapper with core * remove inset from toolbar * tests * update mastheadToggle.tsx * fix kebab visibility * fix circular dependency * PR feedback from mcoker, mattnolting * visibility comment from mcoker

nicolethoen reviewed Jun 7, 2022

View reviewed changes

jenny-s51 added 4 commits June 7, 2022 15:10

align dashboardwrapper with core

2088cf5

remove inset from toolbar

835c62e

tests

eb0b415

update mastheadToggle.tsx

1eb7785

jenny-s51 commented Jun 7, 2022

View reviewed changes

jenny-s51 mentioned this pull request Jun 7, 2022

Chore: Page - Update Masthead to match Dashboard Wrapper #7526

Closed

jenny-s51 commented Jun 7, 2022

View reviewed changes

jenny-s51 added the css review label Jun 7, 2022

nicolethoen reviewed Jun 8, 2022

View reviewed changes

jenny-s51 force-pushed the iss7429 branch from 0ab5895 to 1eb7785 Compare June 8, 2022 13:55

jenny-s51 added 2 commits June 8, 2022 09:56

fix kebab visibility

e626ad2

fix circular dependency

3af6c8c

nicolethoen approved these changes Jun 14, 2022

View reviewed changes

nicolethoen requested review from kmcfaul and mattnolting June 14, 2022 20:34

thatblindgeye approved these changes Jun 15, 2022

View reviewed changes

kmcfaul approved these changes Jun 15, 2022

View reviewed changes

mcoker reviewed Jun 15, 2022

View reviewed changes

mcoker mentioned this pull request Jun 21, 2022

Masthead - keep default/dynamic viewport based inset when using resize observer patternfly/patternfly#4916

Closed

jenny-s51 added 2 commits June 21, 2022 15:32

PR feedback from mcoker, mattnolting

d9a7ee6

visibility comment from mcoker

aa8812c

mcoker approved these changes Jun 21, 2022

View reviewed changes

jenny-s51 added css approved and removed css review labels Jun 22, 2022

nicolethoen approved these changes Jun 22, 2022

View reviewed changes

kmcfaul approved these changes Jun 22, 2022

View reviewed changes

kmcfaul merged commit 2a6fec9 into patternfly:main Jun 22, 2022

	<ToolbarItem visibility={{ default: 'visible', sm: 'visible', md: 'visible', lg: 'hidden', xl: 'hidden' }}>
	<ToolbarItem visibility={{ lg: 'hidden' }}>

chore(DashboardHeader): align DashboardHeader with core #7518

chore(DashboardHeader): align DashboardHeader with core #7518

Uh oh!

Conversation

jenny-s51 commented Jun 6, 2022

Uh oh!

patternfly-build commented Jun 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolethoen left a comment

Choose a reason for hiding this comment

Uh oh!

jenny-s51 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jenny-s51 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolethoen commented Jun 8, 2022

Uh oh!

nicolethoen left a comment

Choose a reason for hiding this comment

Uh oh!

mcoker Jun 15, 2022

Choose a reason for hiding this comment

Uh oh!

mattnolting Jun 21, 2022

Choose a reason for hiding this comment

Uh oh!

mcoker Jun 15, 2022

Choose a reason for hiding this comment

Uh oh!

mcoker Jun 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jenny-s51 Jun 21, 2022

Choose a reason for hiding this comment

Uh oh!

mcoker left a comment

Choose a reason for hiding this comment

Uh oh!

kmcfaul left a comment

Choose a reason for hiding this comment

Uh oh!

patternfly-build commented Jun 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

patternfly-build commented Jun 6, 2022 •

edited

Loading

jenny-s51 left a comment •

edited

Loading

jenny-s51 left a comment •

edited

Loading

mcoker Jun 15, 2022 •

edited

Loading