feat(page): use subgrid to align masthead to page by srambach · Pull Request #6870 · patternfly/patternfly

srambach · 2024-07-10T02:55:16Z

This uses subgrid to align the 3 parts of the masthead to the page sidebar and main container. Fixes #6420

Question: Do we want the space along the sides to be larger on xl breakpoint? That was built in but we don't have a token for that. Should we just use the page inset spacer token for all breakpoints? @lboehling

Question: Is it ok for a divider in the navigation to be the width of the nav items rather than all the way across? @lboehling

Note: If the logo is wide, it will force the sidebar column to be wider

Issue: The grid is still giving too much space to the toggle as a result of (navigation - logo) > toggle

patternfly-build · 2024-07-10T02:59:05Z

Preview: https://patternfly-pr-6870.surge.sh

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

srambach · 2024-07-10T03:01:02Z

Showing scroll in sidebar and divider:

mattnolting · 2024-07-16T14:57:57Z

Through debugging, I found that --pf-v6-c-page__sidebar--Width alters grid column fitting adversely. This example demonstrates how grid column definitions (min-content minmax(200px, max-content) 1fr) can be used to manage sidebar width, with or without the inclusion of masthead__main or the logo therein.

srambach · 2024-07-16T17:09:17Z

Through debugging, I found that --pf-v6-c-page__sidebar--Width alters grid column fitting adversely. This example demonstrates how grid column definitions (min-content minmax(200px, max-content) 1fr) can be used to manage sidebar width, with or without the inclusion of masthead__main or the logo therein.

I think what you are saying is to use this?

.pf-v6-c-page {
    grid-template-columns: min-content minmax(200px, max-content) 1fr;
}

This will make the sidebar no less than 200px - so we need to know what the min desired width is. @lboehling ?
It will also cause the sidebar to grow with long menu items. I wonder if that's ok?

If we use grid-template-columns: min-content minmax(200px, min-content) 1fr; I think it might be a little better?

WDYT? @mattnolting @mcoker

mattnolting · 2024-07-16T17:36:47Z

I think what you are saying is to use this?

.pf-v6-c-page {
    grid-template-columns: min-content minmax(200px, max-content) 1fr;
}

@srambach Yes, also removing width from page__sidebar

grid-template-columns: min-content minmax(200px, min-content) 1fr;

I think it might be a little better?

I agree, @lboehling what should the min-width of the sidebar be?

mattnolting · 2024-07-16T17:54:20Z

@srambach I also modified page layout.

Gave page gap property and padding

:root {
  --pf-v6-c-page--Gap: var(--pf-t--global--spacer--lg);
  --pf-v6-c-page--PaddingInline: var(--pf-t--global--spacer--lg);
  --pf-v6-c-page--ColumnGap: var(--pf-t--global--spacer--lg);
}

Added padding to page

.pf-v6-c-page {
  padding-inline: var(--pf-v6-c-page--PaddingInline);
}

Removed margins from nav, masthead, and page elements and removed grid-areas. This approach was more understandable to me, although you could stick with grid-area.

.pf-v6-c-nav,
.pf-v6-c-page > .pf-v6-c-masthead,
.pf-v6-c-page > .pf-v6-c-masthead .pf-v6-c-masthead__brand,
.pf-v6-c-page__sidebar,
.pf-v6-c-page__main-container {
  --pf-v6-c-page__main-container--MarginInlineStart: 0;
  --pf-v6-c-page__main-container--MarginInlineEnd: 0;

  padding-inline: 0; // remove padding
  margin-inline: 0; // remove padding
  grid-area: unset;
}

Added border to masthead__main in favor of using divider

// masthead border
.pf-v6-c-masthead__main {
  position: relative;
  border: none;
}

.pf-v6-c-masthead__main::before {
  content: "";
  inset-block: 100%;
  inset-inline: calc(var(--pf-v6-c-page--PaddingInline) * -1);
  position: absolute;
  border-bottom: 1px solid gray;
}

Removed width from and added negative margin to sidebar, then applied the same inline padding to sidebar__body to accommodate scrolling for the entire sidebar.

.pf-v6-c-page__sidebar {
  width: unset;
  margin-inline-start: calc(var(--pf-v6-c-page--PaddingInline) * -1);
  margin-inline-end: calc(var(--pf-v6-c-page--ColumnGap) / 2);
}

.pf-v6-c-page__sidebar-body {
  padding-inline-start: var(--pf-v6-c-page--PaddingInline);
  padding-inline-end: calc(var(--pf-v6-c-page--ColumnGap) / 2);
}

Updated grid-template-columns and column spans for tablet layout

@media screen and (min-width: 768px) {
  .pf-v6-c-page {
    grid-template-columns: min-content minmax(200px, max-content) 1fr;
  }

  .pf-v6-c-page__sidebar {
    grid-column: 1 / 3;
  }

  .pf-v6-c-page > .pf-v6-c-masthead,
  .pf-v6-c-page__main-container {
    grid-column: 1 / 4;
  }

  .pf-v6-c-page > .pf-v6-c-masthead {
    grid-template-columns: subgrid;
    column-gap: inhert; // or whatever value needed
  }

  .pf-v6-c-page > .pf-v6-c-masthead .pf-v6-c-masthead__main::before {
    display: none; // this applies to desktop presentation
  }
}

Updated grid-template-columns and column spans for desktop layout

@media screen and (min-width: 1200px) {
  .pf-v6-c-page > .pf-v6-c-masthead .pf-v6-c-masthead__main {
    grid-column-start: 2;
  }

  .pf-v6-c-page > .pf-v6-c-masthead {
    grid-column-start: 1;
    grid-column-end: 5;
  }

  .pf-v6-c-page__main-container {
    grid-column-start: 3;
    grid-column-end: 5;
  }
}

srambach · 2024-07-17T15:35:52Z

This update does a few things - it's a little confusing because names were reused so I'll try to spell it out:

Wraps the toggle and what used to be main in a container and names that container main

masthead
  __toggle
  __main
    a.__brand
      // logo file (masthead-logo.hbs)
  __content

updated to:
masthead
  __main
    __toggle
    __brand
      a.__logo
	// svg (in masthead-image.hbs)
  __content

So -

the SVG in masthead-logo.hbs was renamed to be masthead-image.hbs
__brand was renamed to be __logo
__main was renamed to be __brand
Then wraps __toggle and __brand in __main

Also

Removes the adjustment to the page margin for xl
Removes a couple of old modifiers in the hbs

mcoker · 2024-07-17T18:30:34Z

src/patternfly/components/Masthead/masthead.scss

+.#{$masthead}__logo {
+  align-items: var(--#{$masthead}__logo--AlignItems, var(--#{$masthead}--AlignItems));
+  max-height: var(--#{$masthead}__logo--MaxHeight);
+  margin-inline-end: var(--#{$masthead}__logo--MarginInlineEnd);


A style like this will keep any kind of image from growing larger than this container.

> * { max-width: 100%; max-height: 100%; vertical-align: middle; }

IMO we should not make __logo a flex parent, especially if we're relying on align-items: stretch because it can cause the image to stretch. Here is what happens if you put an image in this box now that's super wide - the height is stretching from the parent's flex align-items

The vertical-align property above will be needed if __logo isn't a flex layout since by default, images have a small gap underneath them to account for characters that have a tail (like j, p, q, y, etc).

mcoker · 2024-07-17T18:34:55Z

src/patternfly/components/Masthead/masthead.scss

+.#{$masthead}__logo {
+  align-items: var(--#{$masthead}__logo--AlignItems, var(--#{$masthead}--AlignItems));
+  max-height: var(--#{$masthead}__logo--MaxHeight);
+  margin-inline-end: var(--#{$masthead}__logo--MarginInlineEnd);


Do we need this margin?

I think we need space there so if there's something left aligned in the content area it doesn't butt against - but I will move it to masthead__main

Nice, that's a good place for it 👍

mcoker · 2024-07-17T18:40:34Z

src/patternfly/components/Masthead/masthead.scss

+.#{$masthead}__logo,
 .#{$masthead}__content {
+  display: flex;
+  align-self: stretch;


I might change this to align-self: center just for __logo. This will center the image relative to the toggle and make __logo hug the image instead of potentially being taller than the image if the image is shorter than __logo

before (with the logo height adjusted to 25px instead of 37px)

after

mcoker

Looks pretty good to me. Want to review the visual regression screenshots. And not a result of this PR, but looks like we're missing --#{$page}__sidebar--PaddingInlineEnd, can you add it back and set it to "0" like --#{$page}__sidebar--PaddingInlineStart below?

patternfly/src/patternfly/components/Page/page.scss

Line 27 in 16feb4b

--#{$page}__sidebar--PaddingInlineStart: 0;

mcoker · 2024-07-23T20:16:53Z

src/patternfly/components/Masthead/masthead.hbs

-  {{~#ifEquals masthead--ColorVariant "light"}}
-    pf-m-light
-  {{else ifEquals masthead--ColorVariant "light-200"}}
-    pf-m-light-200
-  {{/ifEquals}}


mcoker · 2024-07-23T20:44:22Z

src/patternfly/components/Masthead/masthead.scss

+  --#{$masthead}--m-display-inline__brand--GridColumn: unset; 
+  --#{$masthead}--m-display-inline__brand--Order: unset;


This may work but unset is a special keyword that applies to the property its being used on, and in this case the properties are the vars. You can see here that --d: unset does something different than display: unset https://codepen.io/mcoker/pen/NWZrmWb

I would probably do it this way:

Suggested change

--#{$masthead}--m-display-inline__brand--GridColumn: unset;

--#{$masthead}--m-display-inline__brand--Order: unset;

--#{$masthead}--m-display-inline__brand--GridColumn: initial;

--#{$masthead}--m-display-inline__brand--Order: initial;

Then update the vars here to add revert as a fallbacks

patternfly/src/patternfly/components/Masthead/masthead.scss

Line 173 in 16feb4b

grid-column: var(--#{$masthead}__brand--GridColumn);

patternfly/src/patternfly/components/Masthead/masthead.scss

Line 176 in 16feb4b

order: var(--#{$masthead}__brand--Order);

mcoker · 2024-07-23T20:46:13Z

src/patternfly/components/Nav/nav.scss

+  --#{$nav}--PaddingInlineStart: 0; // var(--pf-t--global--spacer--md);
+  --#{$nav}--PaddingInlineEnd: 0; // var(--pf-t--global--spacer--md);


nit to remove comments

Suggested change

--#{$nav}--PaddingInlineStart: 0; // var(--pf-t--global--spacer--md);

--#{$nav}--PaddingInlineEnd: 0; // var(--pf-t--global--spacer--md);

--#{$nav}--PaddingInlineStart: 0;

--#{$nav}--PaddingInlineEnd: 0;

mcoker · 2024-07-23T20:48:28Z

src/patternfly/components/Nav/nav.scss

+  &.pf-m-inset {
+    --#{$nav}--PaddingInlineStart: var(--pf-t--global--spacer--md);
+    --#{$nav}--PaddingInlineEnd: var(--pf-t--global--spacer--md);  


did you mean to leave this in? If so we should document it and make sure it's in react. Though we might chat about the name - I think so far, .pf-m-inset has a size modifier, or it's .pf-m-page-insets, and sometimes we have .pf-m-padding if there is just a single padding value someone would want to apply.

Nah, I'm fine without it if we don't need it!

mcoker · 2024-07-23T20:55:54Z

src/patternfly/components/Page/page.scss

  grid-area: sidebar;
  grid-row-start: 2;
  grid-column-start: 1;
-  width: var(--#{$page}__sidebar--Width);


We'll want to keep this around and used up until the xl breakpoint so that it doesn't consume the viewport width when expanded, then we can just set it to auto or something at the xl breakpoint

I don't think we even need to change it at xl because the grid column is set to the same value.

mcoker · 2024-07-23T20:58:23Z

src/patternfly/components/Page/page.scss

  // Sidebar body
-  --#{$page}__sidebar-body--PaddingInlineEnd: 0;
-  --#{$page}__sidebar-body--PaddingInlineStart: 0;
+  --#{$page}__sidebar-body--PaddingInlineEnd: var(--pf-t--global--spacer--gutter--default);


curious why we wouldn't use var(--pf-t--global--spacer--inset--page-chrome) here, too, to match --#{$page}__sidebar-body--PaddingInlineStart?

Good question; I think it's just left over from rearranging things different ways.

srambach · 2024-07-24T15:39:33Z

Backstop report: https://drive.google.com/file/d/1bno-YUkQrhsDxSQ7eL1DLdDAaHGoboAZ/view?usp=sharing

mcoker

LPTM! Updated this PR with the visual regression screenshots. Here's the report
BackstopJS Report.pdf

patternfly-build · 2024-07-25T01:10:02Z

🎉 This PR is included in version 6.0.0-alpha.202 🎉

The release is available on:

Your semantic-release bot 📦🚀

srambach linked an issue Jul 10, 2024 that may be closed by this pull request

Page/masthead logo - should be the same size as the sidebar/vertical nav #6420

Closed

srambach force-pushed the 6420-masthead-subgrid branch from 77aa949 to 0360296 Compare July 15, 2024 20:28

srambach marked this pull request as ready for review July 16, 2024 13:28

srambach requested review from mattnolting and mcoker July 17, 2024 17:51

mcoker reviewed Jul 17, 2024

View reviewed changes

mcoker mentioned this pull request Jul 22, 2024

Masthead - updates to support better logo layout patternfly/patternfly-react#10788

Closed

srambach force-pushed the 6420-masthead-subgrid branch from 0972f78 to 16feb4b Compare July 23, 2024 19:50

mcoker requested changes Jul 23, 2024

View reviewed changes

srambach added 12 commits July 24, 2024 09:22

feat(page): rebase

9c91cd9

feat(page): remove extra col track at md breakpoint

705f59b

feat(page): move sidebar width to grid and remove modifier

a65c026

feat(page): adjust sidebar width

9f02537

feat(page): don't subgrid until xl breakpoint

7e75a74

feat(page): rearrange masthead to wrap toggle and brand

947edd6

feat(page): working on logo sizing

afc40e7

feat(page): attempt to limit logo size

d023a1a

feat(page): clean up grid containers

863fc08

feat(page): explicit width on masthead logo container

45734cf

feat(page): resolve conflict and remove test images

4d257e9

feat(page): address comments

3f57196

feat(page): fix colgap in masthead stack display

561d032

srambach force-pushed the 6420-masthead-subgrid branch from 2c71b33 to 561d032 Compare July 24, 2024 18:43

chore(backstop): screenshots

ecfb34b

mcoker approved these changes Jul 25, 2024

View reviewed changes

mcoker merged commit 8fe1357 into patternfly:v6 Jul 25, 2024

mcoker mentioned this pull request Jul 25, 2024

Page/masthead logo - should be the same size as the sidebar/vertical nav #6420

Closed

patternfly-build added the released on @alpha label Jul 25, 2024

tlabaj assigned tlabaj and unassigned tlabaj Jul 25, 2024

patternfly-build mentioned this pull request Apr 16, 2025

The automated release is failing 🚨 #7464

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(page): use subgrid to align masthead to page#6870

feat(page): use subgrid to align masthead to page#6870
mcoker merged 14 commits intopatternfly:v6from
srambach:6420-masthead-subgrid

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

		--#{$masthead}--m-display-inline__brand--GridColumn: unset;
		--#{$masthead}--m-display-inline__brand--Order: unset;

		--#{$nav}--PaddingInlineStart: 0; // var(--pf-t--global--spacer--md);
		--#{$nav}--PaddingInlineEnd: 0; // var(--pf-t--global--spacer--md);

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants