docs(Page): Update examples and demos to use Masthead #6388

wise-king-sullyman · 2021-10-01T00:49:53Z

What: Closes #6281

Additional issues: N/A

Refactors the Page examples to use Masthead and Toolbar rather than PageHeader and PageHeaderTools. One Page example using PageHeader was kept but relocated to be the last example.

A new Page demo was added using Masthead, the other demos were renamed to clarify that they're using PageHeader.

patternfly-build · 2021-10-01T01:01:06Z

PF4 preview: https://patternfly-react-pr-6388.surge.sh

mcarrano

@wise-king-sullyman So is it only the first demo then that was refactored to use the new masthead? In any case, the masthead utility items should be updated to match the updated Navigation demos. Here's what that should look like:

And also see this: https://patternfly-react-pr-6388.surge.sh/components/navigation/react-demos/default-nav/

wise-king-sullyman · 2021-10-01T13:42:24Z

@mcarrano Yes only the first demo was refactored to use Masthead, the others I left to demonstrate PageHeader since it's still in use. If you would like me to get rid of/refactor all of the PageHeader demos (or two of the three) though I'm happy to do so.

I will update the utility items shortly 👍

mcarrano · 2021-10-01T14:09:44Z

@wise-king-sullyman I think it's OK to leave the other demos as is and just update the first one with the new utility items.

wise-king-sullyman · 2021-10-01T14:51:47Z

While doing requested updates I also updated demo screenshots and removed unneeded toolbar groups from the page examples to make them cleaner.

nicolethoen

Could you add href="#" to each of the navItems in the demos - so that when the user tabs through the page, it will grant access to the tertiary nav items?

redallen

Thanks for updating the demo thumbnails! I'm a bit confused about what we're doing with the old PageHeader now.

packages/react-core/src/components/Page/examples/Page.md

packages/react-core/src/demos/Page/Page.md

mcarrano

Looks great. Thanks for making those changes.

evwilkin · 2021-10-02T00:48:45Z

(FYI deleted my comment, not able to reproduce issue anymore that only I was seeing.)

redallen

Thanks @wise-king-sullyman !

mattnolting

Looks great! The only issue I see is icon-button-group grouping.

mattnolting · 2021-10-04T15:54:12Z

packages/react-core/src/demos/Page/Page.md

+          <ToolbarGroup
+            variant="icon-button-group"
+            alignment={{ default: 'alignRight' }}
+            spacer={{ default: 'spacerNone', md: 'spacerMd' }}
+          >
+            <ToolbarItem>
+              <Button aria-label="Notifications" variant={ButtonVariant.plain}>
+                <AttentionBellIcon />
+              </Button>
+            </ToolbarItem>
+            <ToolbarGroup variant="icon-button-group" visibility={{ default: 'hidden', lg: 'visible' }}>
+              <ToolbarItem>
+                <Button aria-label="Settings actions" variant={ButtonVariant.plain}>
+                  <CogIcon />
+                </Button>
+              </ToolbarItem>
+              <ToolbarItem>
+                <Button aria-label="Help actions" variant={ButtonVariant.plain}>
+                  <HelpIcon />
+                </Button>
+              </ToolbarItem>
+            </ToolbarGroup>
+          </ToolbarGroup>
+          <ToolbarItem visibility={{ default: 'hidden', md: 'visible', lg: 'hidden' }}>
+            <Dropdown
+              isPlain
+              position="right"
+              onSelect={this.onKebabDropdownSelect}
+              toggle={<KebabToggle onToggle={this.onKebabDropdownToggle} />}
+              isOpen={isKebabDropdownOpen}
+              dropdownItems={kebabDropdownItems}
+            />
+          </ToolbarItem>
+          <ToolbarItem visibility={{ default: 'visible', md: 'hidden', lg: 'hidden', xl: 'hidden', '2xl': 'hidden' }}>
+            <Dropdown
+              isPlain
+              position="right"
+              onSelect={this.onFullKebabSelect}
+              toggle={<KebabToggle onToggle={this.onFullKebabToggle} />}
+              isOpen={isFullKebabDropdownOpen}
+              dropdownItems={fullKebabItems}
+            />
+          </ToolbarItem>


All of the dropdowns/icon buttons go in the same base icon-button-group for proper spacing.

<ToolbarGroup variant="icon-button-group" alignment={{ default: 'alignRight' }} spacer={{ default: 'spacerNone', md: 'spacerMd' }} > <ToolbarItem>Bell</ToolbarItem> <ToolbarItem>Cog</ToolbarItem> <ToolbarItem>Help</ToolbarItem> <ToolbarItem>Dropdown</ToolbarItem> <ToolbarItem>Dropdown</ToolbarItem> </ToolbarGroup>

Then place the user dropdown outside of the icon-button-group

<ToolbarItem visibility={{ default: 'hidden', md: 'visible' }}> <Dropdown position="right" onSelect={this.onDropdownSelect} isOpen={isDropdownOpen} toggle={ <DropdownToggle icon={<Avatar src={imgAvatar} alt="Avatar" />} onToggle={this.onDropdownToggle}> John Smith </DropdownToggle> } dropdownItems={userDropdownItems} /> </ToolbarItem>

I actually copied that from DashboardHeader.js want me to go ahead and fix it there too? Or since that's not directly related would it be too out of scope for this refactor?

For now, I'll just fix it here. @mattnolting if you would like this fixed in DashboardHeader under this PR as well just let me know and I'll add it in.

mattnolting · 2021-10-04T21:05:35Z

I actually copied that from DashboardHeader.js want me to go ahead and fix it there too? Or since that's not directly related would it be too out of scope for this refactor?

@wise-king-sullyman Oh, I see that now. It's a bit off in dashboard header as is. Toolbar groups and items can be nested infinitely, so having an icon-button-group inside of an icon-button-group is perfectly fine, as the purpose is to hide the entire group, rather than individual items within. That's what DashboardHeader.js is doing. But the last, user avatar dropdown should be outside of any icon-button-group resulting in added spacing between the row of icons and user avatar dropdown.

For now, I'll just fix it here. @mattnolting if you would like this fixed in DashboardHeader under this PR as well just let me know and I'll add it in.

You could just fix here and we'll create a followup. Maybe DashboardHeader.js update could highlight its current implementation, with the user avatar dropdown outside, rather than inside, of the icon-button-group as an alternative ;)

packages/react-core/src/demos/Page/Page.md

Co-authored-by: Matt Nolting <matthew.nolting@gmail.com>

wise-king-sullyman · 2021-10-04T23:10:41Z

Just realized that the three-dot dropdown isn't supposed to show at the same time as the help icon and settings gear, because it contains the help icon and settings gear. Also your comment about the nested group being allowed, and that the purpose is to hide those items together just clicked with me.

I'm thinking the proper course of action moving forward is to add the nested toolbar group back in around the help and gear icons, with the appropriate visibility settings, and just keep the non-user dropdowns in the first tier of ToolbarGroups with the user dropdown outside of any ToolbarGroups.

Is that correct/what you're looking for @mattnolting ? I figure it's worth checking in with you this time before I just assume that I'm understanding you correctly again.

mattnolting · 2021-10-04T23:48:31Z

@wise-king-sullyman

Just realized that the three-dot dropdown isn't supposed to show at the same time as the help icon and settings gear, because it contains the help icon and settings gear. Also your comment about the nested group being allowed, and that the purpose is to hide those items together just clicked with me.

Awesome! 💯

I'm thinking the proper course of action moving forward is to add the nested toolbar group back in around the help and gear icons, with the appropriate visibility settings, and just keep the non-user dropdowns in the first tier of ToolbarGroups with the user dropdown outside of any ToolbarGroups.

Yep, you got it!

Is that correct/what you're looking for @mattnolting ? I figure it's worth checking in with you this time before I just assume that I'm understanding you correctly again.

Perfect 👍

This reverts commit dfd8d51.

mattnolting

LPTM! Excellent work @wise-king-sullyman 💎

mcarrano

Looks good to me, too. Thanks @wise-king-sullyman !

kmcfaul · 2021-10-05T14:33:29Z

packages/react-core/src/components/Page/examples/Page.md

-      headerTools={<PageHeaderTools>header-tools</PageHeaderTools>}
-      topNav="Navigation"
-    />
+    <Masthead display={{ default: 'stack' }} inset={{ default: 'insetXs' }}>


I don't think the forced stack display is needed here. The horizontal page indicates the navigation is inside the toolbar rather than in a separate sidebar.

docs(Page): Update examples and demos to use Masthead

df6d525

wise-king-sullyman requested review from mcarrano, mcoker, nicolethoen and tlabaj October 1, 2021 02:16

mcarrano requested changes Oct 1, 2021

View reviewed changes

wise-king-sullyman added 3 commits October 1, 2021 10:29

update new page demo to use updated utility items from Nav demo

bb7f40b

update demo screenshots

4dcec5f

remove unneeded toolbar groups from page examples

8dbe002

wise-king-sullyman requested a review from mcarrano October 1, 2021 15:36

nicolethoen requested changes Oct 1, 2021

View reviewed changes

redallen suggested changes Oct 1, 2021

View reviewed changes

packages/react-core/src/components/Page/examples/Page.md Show resolved Hide resolved

packages/react-core/src/demos/Page/Page.md Show resolved Hide resolved

packages/react-core/src/demos/Page/Page.md Show resolved Hide resolved

wise-king-sullyman added 2 commits October 1, 2021 13:51

make demo navitems tab navigable

03257f0

fix toolbaritem indentation

1f11e92

mcarrano previously approved these changes Oct 1, 2021

View reviewed changes

add note clarifying that pageheader shouldn't be used going forward

7a3fddf

wise-king-sullyman dismissed mcarrano’s stale review via 7a3fddf October 1, 2021 21:16

wise-king-sullyman requested review from nicolethoen, mcarrano and redallen October 1, 2021 21:33

redallen previously approved these changes Oct 4, 2021

View reviewed changes

mcoker requested a review from mattnolting October 4, 2021 14:22

mattnolting requested changes Oct 4, 2021

View reviewed changes

Remove nested toolbargroup

dfd8d51

wise-king-sullyman dismissed redallen’s stale review via dfd8d51 October 4, 2021 20:04

wise-king-sullyman requested a review from mattnolting October 4, 2021 20:37

mattnolting reviewed Oct 4, 2021

View reviewed changes

packages/react-core/src/demos/Page/Page.md Outdated Show resolved Hide resolved

Move non-user dropdowns into toolbargroup

2e5ef12

Co-authored-by: Matt Nolting <matthew.nolting@gmail.com>

Revert "Remove nested toolbargroup"

21b6598

This reverts commit dfd8d51.

wise-king-sullyman requested a review from mattnolting October 5, 2021 02:44

mattnolting approved these changes Oct 5, 2021

View reviewed changes

wise-king-sullyman requested a review from redallen October 5, 2021 13:03

mcarrano approved these changes Oct 5, 2021

View reviewed changes

nicolethoen approved these changes Oct 5, 2021

View reviewed changes

redallen merged commit dc0370a into patternfly:main Oct 5, 2021

kmcfaul reviewed Oct 5, 2021

View reviewed changes

wise-king-sullyman deleted the refactor-page-examples-and-demos branch October 5, 2021 14:50

wise-king-sullyman mentioned this pull request Oct 5, 2021

docs(page): update horizontal nav example to not be forced stacked #6402

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs(Page): Update examples and demos to use Masthead #6388

docs(Page): Update examples and demos to use Masthead #6388

wise-king-sullyman commented Oct 1, 2021

patternfly-build commented Oct 1, 2021 •

edited

Loading

mcarrano left a comment

wise-king-sullyman commented Oct 1, 2021 •

edited

Loading

mcarrano commented Oct 1, 2021

wise-king-sullyman commented Oct 1, 2021

nicolethoen left a comment

redallen left a comment

mcarrano left a comment

evwilkin commented Oct 2, 2021

redallen left a comment

mattnolting left a comment

mattnolting Oct 4, 2021

wise-king-sullyman Oct 4, 2021 •

edited

Loading

mattnolting commented Oct 4, 2021

wise-king-sullyman commented Oct 4, 2021

mattnolting commented Oct 4, 2021

mattnolting left a comment

mcarrano left a comment

kmcfaul Oct 5, 2021

docs(Page): Update examples and demos to use Masthead #6388

docs(Page): Update examples and demos to use Masthead #6388

Conversation

wise-king-sullyman commented Oct 1, 2021

patternfly-build commented Oct 1, 2021 • edited Loading

mcarrano left a comment

Choose a reason for hiding this comment

wise-king-sullyman commented Oct 1, 2021 • edited Loading

mcarrano commented Oct 1, 2021

wise-king-sullyman commented Oct 1, 2021

nicolethoen left a comment

Choose a reason for hiding this comment

redallen left a comment

Choose a reason for hiding this comment

mcarrano left a comment

Choose a reason for hiding this comment

evwilkin commented Oct 2, 2021

redallen left a comment

Choose a reason for hiding this comment

mattnolting left a comment

Choose a reason for hiding this comment

mattnolting Oct 4, 2021

Choose a reason for hiding this comment

wise-king-sullyman Oct 4, 2021 • edited Loading

Choose a reason for hiding this comment

mattnolting commented Oct 4, 2021

wise-king-sullyman commented Oct 4, 2021

mattnolting commented Oct 4, 2021

mattnolting left a comment

Choose a reason for hiding this comment

mcarrano left a comment

Choose a reason for hiding this comment

kmcfaul Oct 5, 2021

Choose a reason for hiding this comment

patternfly-build commented Oct 1, 2021 •

edited

Loading

wise-king-sullyman commented Oct 1, 2021 •

edited

Loading

wise-king-sullyman Oct 4, 2021 •

edited

Loading