replace Toolbar with PageHeaderTools in page header #4223

jschuler · 2020-05-08T17:07:47Z

What: Closes #4106

This PR updates the page demos to use the new PageHeaderTools/PageHeaderToolsGroup/PageHeaderToolsItem components

https://patternfly-react-pr-4223.surge.sh/documentation/react/demos/pagelayout

Breaking changes

PageHeader: avatar prop was removed. Add the avatar to the PageHeaderTools component instead (which is passed into PageHeader via the headerTools prop.
PageHeader: toolbar prop was removed. Use the headerTools prop instead. Also, if you previously used the Toolbar, ToolbarGroup, or ToolbarItem components for the header, replace them with the PageHeaderTools, PageHeaderToolsGroup, and PageHeaderToolsItem components.

patternfly-build · 2020-05-08T17:16:52Z

PF4 preview: https://patternfly-react-pr-4223.surge.sh
A11y report: https://patternfly-react-pr-4223-coverage.surge.sh

redallen

Looks good, just need some notes

packages/react-core/src/demos/CardView/examples/CardView.md

nicolethoen · 2020-05-08T19:33:52Z

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

 section: components
 cssPrefix: 'pf-c-page'
 typescript: true
 propComponents: ['Page', 'PageHeader', 'PageSidebar', 'PageSection']


Should the new Components be listed here so their props are included in the docs?

dlabrecq

LGTM

mcoker · 2020-05-08T21:29:33Z

packages/react-core/src/components/Page/PageHeaderToolsGroup.tsx

+    className={css(
+      styles.pageHeaderToolsGroup,
+      className,
+      hiddenOnLg && styles.modifiers.mobile,


If we're going to structure this so that we offer header toolbar groups and items that require the user specify visibleOnLg for items that are only visible on desktop, hiddenOnSm for items that are hidden on mobile, etc, then I wonder if we should drop the current modifiers and use a hidden/visible mixin that would give us a set of styles that allow a user to specify pf-m-hidden[-on-{breakpoint}] and pf-m-visible[-on-{breakpoint}] on any header-tools-group or header-tools-item. In this PR, users can only specify hiddenOnLg , visibleOnLg, and hiddenOnSm.

Then users will have full control over hiding/showing things at any breakpoint. The downside to the current approach (whether we include the mixin or not) is that the responsibility is on the user to hide/show header toolbar items appropriately. The alternative to that would be to have something like 3 props on the header tools component, mobileDropdown, desktopActions, and userMenu or something like that, and we put the contents of those props in the appropriate classes and control the mobile, desktop, and user menu breakpoints where that content is hidden/shown.

The former seems like the better approach, but could lead to inconsistency in implementation. @mcarrano wdyt? If we're OK allowing users to hide/show any header toolbar items/groups at any breakpoint, but we show the proper way to do it in the demos and could have a masthead/header design guideline that goes into more detail, I'd like to drop the existing classes and add the full hidden/visible breakpoint classes to header toolbar groups and items.

We could keep the existing classes and add the hidden/visible classes on top, but that seems like it might be confusing to have both, and the prop names that enable the current classes seem like they would overlap.

Yes, I was going back and forth with the prop names, initially I named them something similar to your mentioned alternative before landing on this (as it also seemed familiar with what we have in other components).
I guess if we wanted to we could streamline this even more and provide a way for users to define specific things and we can lay it out for them:

<PageHeaderTools mobileDropdown={<Dropdown />} desktopActions={ { component: <NotificationBadge />, isSelected: false } { component: <Button><CogIcon /></Button>, isSelected: true } } userDropdown={ <Dropdown /> } avatar={<Avatar />} > Other components here that should come before? </PageHeaderTools>

Maybe this for ease-of-use, but still allow users to use the component without those convenience props and define the content themselves using visibility mixins?

Looking at the way it's done in master now, it looks like we import the accessibility utility classes and it's up to users to apply hidden/visible classes on their own. It seems logical then to bake this hidden/visible behavior into the page header tools, and give users the ability to hide/show header elements at any breakpoint.

mcoker · 2020-05-08T21:37:17Z

packages/react-core/src/components/Page/PageHeaderToolsItem.tsx

+ * will render
+ * <button class="pf-m-plain pf-m-selected">Hello</button>
+ */
+export const PageHeaderToolsItem: React.FunctionComponent<PageHeaderToolsItemProps> = ({


I'm curious how this component works. I think it would be more consistent if we had a .pf-c-page__header-tools-item class that you could apply hidden/visible to. I'm assuming that would still allow us to apply .pf-m-selected to a <Button> used in .pf-c-page__header-tools-item if it has the isSelected prop?

So the problem is, in HTML you can just add the pf-m-selected class to a button anywhere. If you wanted to do this in react you'd have to manually add this class name yourself i.e.

<PageHeaderTools> <PageHeaderToolsGroup> <Button className="pf-m-selected"> <CogIcon /> </Button> </PageHeaderToolsGroup> </PageHeaderTools>

because the selected modifier is not a prop we have available for Buttons, the class is defined in the context of page.

Wrapping the <Button> component in <PageHeaderToolsItem> basically allows me to pass the pf-m-selected class or any other potential modifier down to the child component.

mcoker

This looks great, @jschuler! I left a few comments and created a follow up core issue to fix an alignment issue I'm seeing with one of the buttons in the header patternfly/patternfly#3086. Though how we fix that depends on what we decide with the comments I left, so I'll hold off on addressing #3086 for the time being.

mcarrano · 2020-05-11T14:53:18Z

@jschuler I realize that the goal of this PR is not to fully implement the intended design updates for the masthead, but here is a Marvel deck that shows where we want to get to for reference: https://marvelapp.com/5cjajdg/screen/63256946 Getting the demo as close as possible to this would be great. Note the the Notification icon never hides.

jschuler · 2020-05-11T17:18:07Z

@mcarrano thanks for that, I'll update the demo to reflect this layout as much as possible.

@mcarrano @mcoker and @mceledonia and I met this morning and decided that core will make some changes. Core will remove the pf-m-mobile, pf-m-user, and pf-m-icons classes, and instead bring in the breakpoint modifier classes.
Core will also create a pf-c-page__header-tools-item class.

Once these changes are in the core v4 branch, we'll bring it into react and then I'll update this PR. So it'll be on hold until then!

mcoker · 2020-05-11T20:04:25Z

@jschuler patternfly/patternfly#3091 was merged.

Removed .pf-m-[icon/mobile/user]
Adds .pf-c-page__header-tools-item (as a child of .pf-c-page__header-tools-group)
- .pf-c-page_-header-tools-group is still required for any .pf-c-page__header-tools-item
Adds .pf-m-hidden[-on-{breakpoint}] and .pf-m-visible[-on-{breakpoint}] that can be applied to __header-tools-[item/group]

redallen · 2020-05-12T18:01:08Z

Depends on #4243

jschuler · 2020-05-13T18:03:12Z

packages/react-core/src/components/Page/PageHeaderToolsItem.tsx

+  /** Content rendered in page header tools item. If you need the selected class, use the render prop instead */
+  children?: React.ReactNode;
+  /** If you need the selected class, use this render prop instead of children */
+  render?: (props: InjectedPageHeaderToolsItemProps) => React.ReactNode;


opened issue in core so this hopefully won't be necessary to get the selected class
patternfly/patternfly#3108

We haven't really used an API like this before, but I like it and I think it satisfies the documentation part.

Yes, I remembered there was some controversy about using cloneElement so render prop puts the resposibility of adding the class back to the user. anyways, this render prop will be short-lived once the mentioned core PR is available :)

redallen

I think after RC1 we can come back and clean up the render API. One it passes CI this LGTM.

redallen · 2020-05-13T18:07:52Z

packages/react-core/src/components/Page/PageHeaderToolsItem.tsx

+  /** Content rendered in page header tools item. If you need the selected class, use the render prop instead */
+  children?: React.ReactNode;
+  /** If you need the selected class, use this render prop instead of children */
+  render?: (props: InjectedPageHeaderToolsItemProps) => React.ReactNode;


We haven't really used an API like this before, but I like it and I think it satisfies the documentation part.

redallen

LGTM! Thanks @jschuler !

tlabaj · 2020-05-13T19:12:08Z

packages/react-core/src/components/Page/PageHeaderToolsGroup.tsx

+  <div
+    className={css(
+      styles.pageHeaderToolsGroup,
+      breakpointMods && formatBreakpointMods(breakpointMods, styles),


I opened an issue agains the page header. formatBreakpointMods function does not work for the 2xl breakpoint.

mcoker · 2020-05-13T19:15:03Z

This LGTM, my only comments are that the notification bell should be in a header-tools-item element, and that the cog shouldn't be selected. We can add the selected state as an example with either a drawer or notification drawer demo where a header tools button toggles a drawer.

mcarrano

This is looking good @jschuler . I just had a couple of requests to try and get this closer to the intended design.

At the medium breakpoint the avatar should hide and the user dropdown just displays the user name as shown here:

At the small breakpoint, the user menu should collapse entirely into the kebob as shown here:

The way it is now, the user menu is not accessible when only the avatar is present.

Also, I would remove the selected state from the gear icon. Ideally each of those icons would have a hover which is same as the selected state you are showing. Let me know if you have questions. I would be glad to walk through this with you.

codecov-io · 2020-05-14T14:59:49Z

Codecov Report

Merging #4223 into v4 will decrease coverage by 2.49%.
The diff coverage is 42.88%.

@@            Coverage Diff             @@
##               v4    #4223      +/-   ##
==========================================
- Coverage   57.85%   55.36%   -2.50%     
==========================================
  Files         391      428      +37     
  Lines        5979     7077    +1098     
  Branches     2354     2612     +258     
==========================================
+ Hits         3459     3918     +459     
- Misses       2064     2666     +602     
- Partials      456      493      +37

Flag	Coverage Δ
#patternfly4	`55.36% <42.88%> (-2.50%)`	⬇️

Impacted Files	Coverage Δ
...ages/react-core/src/components/Page/PageHeader.tsx	`72.72% <0.00%> (+6.06%)`	⬆️
packages/react-core/src/components/Tabs/Tab.tsx	`100.00% <ø> (ø)`
...kages/react-core/src/components/Tabs/TabButton.tsx	`83.33% <ø> (ø)`
packages/react-core/src/helpers/util.ts	`46.66% <ø> (ø)`
...mponents/Table/utils/decorators/compoundExpand.tsx	`90.00% <ø> (ø)`
...-table/src/components/Table/utils/transformers.tsx	`100.00% <ø> (ø)`
...ackages/react-topology/src/utils/useCallbackRef.ts	`0.00% <0.00%> (ø)`
packages/react-topology/src/elements/BaseEdge.ts	`1.61% <1.61%> (ø)`
packages/react-topology/src/utils/svg-utils.ts	`2.56% <2.56%> (ø)`
packages/react-topology/src/utils/useHover.ts	`3.03% <3.03%> (ø)`
... and 94 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d671a80...ee09981. Read the comment docs.

mcoker · 2020-05-14T15:39:28Z

@jschuler looks like we lost the header tools in the card view demo - https://patternfly-react-pr-4223.surge.sh/documentation/react/demos/cardview/basic. Otherwise LGTM!

jschuler · 2020-05-14T15:45:10Z

@mcoker thanks, I can add those in a followup PR

mcarrano

Thanks for the latest set of updates @jschuler . This looks good now.

tlabaj

Thanks for the updates. Looks good.

redallen · 2020-05-14T16:36:17Z

@jschuler Fix conflicts and we can merge!

replace Toolbar with PageHeaderTools in page header

79f91ff

jschuler requested review from dlabrecq, evwilkin, mcoker and redallen May 8, 2020 17:07

merge

16598dc

jschuler added 9 commits May 8, 2020 13:36

add missing snapshots and fix some tests

f1d4a9b

more instances of toolbar -> headerTools

2f73316

revert 2 snapshots

c3588b2

fix integration spec

b567a41

integration

bf70341

another intergration spec...

7c3f2e2

the spec

9d4e93d

eventually i will get it

6553c26

improve Page examples

892a90d

redallen suggested changes May 8, 2020

View reviewed changes

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

nicolethoen reviewed May 8, 2020

View reviewed changes

dlabrecq approved these changes May 8, 2020

View reviewed changes

mcoker reviewed May 8, 2020

View reviewed changes

jschuler added the on hold label May 11, 2020

jschuler added 3 commits May 13, 2020 09:57

view new components in docs

aaba4f2

merge

7c565a1

change after latest core update

7984c11

item cleanup

a81b5d9

jschuler commented May 13, 2020

View reviewed changes

redallen reviewed May 13, 2020

View reviewed changes

snapshots and md linting

5b28664

jschuler removed the on hold label May 13, 2020

redallen approved these changes May 13, 2020

View reviewed changes

tlabaj added the Breaking change 💥 this change requires a major release and has API changes. label May 13, 2020

tlabaj reviewed May 13, 2020

View reviewed changes

mcarrano suggested changes May 13, 2020

View reviewed changes

jschuler added 3 commits May 14, 2020 09:53

Merge remote-tracking branch 'upstream/v4' into page-header-tools

728f631

update after latest select class changes from core

de00b92

actually use the help icon

ee09981

bump core to latest

2821962

mcoker approved these changes May 14, 2020

View reviewed changes

mcarrano approved these changes May 14, 2020

View reviewed changes

redallen requested a review from tlabaj May 14, 2020 16:28

tlabaj approved these changes May 14, 2020

View reviewed changes

Merge branch 'v4' into page-header-tools

90a90d1

redallen merged commit 3a7ae11 into patternfly:v4 May 14, 2020

redallen mentioned this pull request May 19, 2020

Add use-page-header-tools patternfly/pf-codemods#27

Closed

2 tasks

replace Toolbar with PageHeaderTools in page header #4223

replace Toolbar with PageHeaderTools in page header #4223

Uh oh!

Conversation

jschuler commented May 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Breaking changes

Uh oh!

patternfly-build commented May 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

redallen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dlabrecq left a comment

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!

mcoker left a comment

Choose a reason for hiding this comment

Uh oh!

mcarrano commented May 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jschuler commented May 11, 2020

Uh oh!

mcoker commented May 11, 2020

Uh oh!

redallen commented May 12, 2020

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!

redallen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

redallen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcoker commented May 13, 2020

Uh oh!

mcarrano left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-io commented May 14, 2020

Codecov Report

Uh oh!

mcoker commented May 14, 2020

Uh oh!

jschuler commented May 14, 2020

Uh oh!

mcarrano left a comment

Choose a reason for hiding this comment

Uh oh!

tlabaj left a comment

Choose a reason for hiding this comment

Uh oh!

redallen commented May 14, 2020

Uh oh!

Reviewers

Assignees

jschuler commented May 8, 2020 •

edited

Loading

patternfly-build commented May 8, 2020 •

edited

Loading

mcarrano commented May 11, 2020 •

edited

Loading