feat(Popover): add composable header and alert variants by kmcfaul · Pull Request #6664 · patternfly/patternfly-react

kmcfaul · 2021-12-01T18:23:26Z

What: Closes #6267

Adds header as an alternative, composable way to build a popover header
Adds PopoverHeader, PopoverHeaderIcon, PopoverHeaderText components to build headers
Adds alertVariant prop to Popover to control alert styling
Renames the previous PopoverHeader to PopoverInternalHeader. This component wasn't exported before so this shouldn't be breaking.

After some review, the initial approach was altered. Now:

Adds alertSeverityVariant and alertSeverityScreenReaderText prop to Popover to control alert styling
Adds PopoverHeader, PopoverHeaderIcon, PopoverHeaderText components to build headers (not exposed)
Adds headerIcon
When headerIcon or alertSeverityVariant are defined, the headerContent is placed in PopoverHeaderText rather than a Title component, and the whole header is wrapped in a header element.

patternfly-build · 2021-12-01T18:28:21Z

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

mcarrano

This looks great. Thanks @kmcfaul !

nicolethoen

VO is not announcing the alerts.
I think that @evwilkin has opened a PR to address that? I think if his gets merged first, then we should rebase and double check it.

jschuler · 2021-12-03T16:53:46Z

-      </Title>
-    )}
-  </PopoverContext.Consumer>
+  <header className={css('pf-c-popover__header')} {...props}>


can you add className as a prop and merge it in here, otherwise it would override this one if specified by consumer
Also applies to the other components in this PR

tlabaj · 2021-12-07T16:12:45Z

 import { css } from '@patternfly/react-styles';

+export interface PopoverFooterProps extends React.HTMLProps<HTMLDivElement> {
+  /** Additional classes added to the Popover Footer */


nit. sentence case

tlabaj · 2021-12-07T16:38:46Z

+   */
+  header?: React.ReactNode | ((hide: () => void) => React.ReactNode);
+  /** Variants for an alert popover */
+  alertVariant?: 'default' | 'info' | 'warning' | 'success' | 'danger';


So, I think the alertVariant on its own does not make sense. Setting this prop does not make the popover an alert popover. I wonder if we can improve on the API?
@jschuler I would be interested in your thoughts.

@tlabaj how about alertLevelVariant or alertSeverityVariant

@nicole, those suggestions are good. Maybe some more context in the description of what it does will also help.

It would make more sense for me to find a prop like that on the PopoverHeader component, since it affects the icon and text color in there. But if we anticipate more changes to the entire Popover it would make sense to be here.

If the prop comment explains that it modifies the header icon and text colors, that would make it clearer what change it does.

Edit: Saw the change to the PR and LGTM

tlabaj · 2021-12-07T18:52:34Z

  headerContent?: React.ReactNode | ((hide: () => void) => React.ReactNode);
  /** Sets the heading level to use for the popover header. Default is h6. */
  headerComponent?: 'h1' | 'h2' | 'h3' | 'h4' | 'h5' | 'h6';
+  /** @beta Composable header component. Use with the PopoverHeader, PopoverHeaderText, and PopoverHeaderIcon components. To manually close


Beat me to it. I was going to ask for beta tag :)

tlabaj

Can you also mark new exported componets Beta please.

wise-king-sullyman · 2021-12-07T21:55:46Z

+  /** ID of the popover header. */
+  id?: string;


Should id still be in this interface? It doesn't look like it's used in the component anymore.

mcoker · 2021-12-08T16:20:17Z

-    )}
-  </PopoverContext.Consumer>
+  <header className={css('pf-c-popover__header', className)} {...props}>
+    <h1 className={css(styles.popoverTitle, hasIcon && styles.modifiers.icon)}>{children}</h1>


Should headerComponent set the heading level here, too.

I'm not sure - I've been trying to figure out the real differences between PopoverHeader and PopoverInternalHeader and one of the main differences is that PopoverInternalHeader uses a Title component. @mcoker Do you think they should both use Title components?

@kmcfaul is the only reason you built this to be composable because you needed to put the hasIcon prop at the outermost level?

@mcoker Do you think they should both use Title components?

@nicolethoen no, I think the popover title is better as an element of the popover component instead of the title component. That way we can change the styling (size, weight, etc) of the title in the CSS as opposed to needing a markup change, which is on the user to make in some cases. When we introduced the new Red Hat font, for example, the change in font face made either the modal or popover title (can't remember which, but both used the <Title> component) look different, so we ended up styling .pf-c-title.pf-m-lg (or whatever the class name was) to a different font-size in that component, which is a bad practice. Having the title be an element of a component won't put us in a situation like that if the title styles need to change.

In our next upcoming breaking change, I'd like to get rid of the <Title> component in the popover and make the header and title elements introduced in this PR the default structural elements for the popover's title.

@mcoker, I think you are right, otherwise the heading level for the composable PopoverHeader is not configurable. We need to add a prop to this component to configure heading level.

tlabaj · 2021-12-08T20:22:38Z

+    <header className={css('pf-c-popover__header', className)} {...props}>
+      <HeadingLevel className={css(styles.popoverTitle, icon && styles.modifiers.icon)}>
+        {icon && <PopoverHeaderIcon>{icon}</PopoverHeaderIcon>}
+        {alertSeverityVariant && <span className="pf-u-screen-reader">{alertSeverityVariant} alert:</span>}


The "alert" text in this span needs to be configurable.

tlabaj · 2021-12-08T20:38:46Z

+            alertSeverityVariant={alertSeverityVariant}
+            titleHeadingLevel={headerComponent}
+          >
+            <React.Fragment>{typeof headerContent === 'function' ? headerContent(hide) : headerContent}</React.Fragment>


Why do we need the <React.Fragment>

tlabaj · 2021-12-08T20:53:29Z

+
+```ts isBeta
+import React from 'react';
+import { Popover, PopoverHeader, PopoverHeaderIcon, PopoverHeaderText, Button } from '@patternfly/react-core';


You can remove PopoverHeaderText, PopoverHeaderIcon and PopoverHeader

tlabaj · 2021-12-08T20:53:53Z

+
+```ts isBeta
+import React from 'react';
+import { Popover, PopoverHeader, PopoverHeaderIcon, PopoverHeaderText, Button } from '@patternfly/react-core';


You can remove PopoverHeaderText, PopoverHeaderIcon and PopoverHeader

adding modifier for icon Co-authored-by: Michael Coker <35148959+mcoker@users.noreply.github.com>

mcoker · 2021-12-08T21:00:26Z

  children,
+  icon,
+  className,
+  titleHeadingLevel = 'h1',


I would probably expect this to be h6 to match the existing headerComponent default value.

tlabaj · 2021-12-08T21:48:47Z

+            icon={headerIcon}
+            alertSeverityVariant={alertSeverityVariant}
+            alertSeverityScreenReaderText={
+              alertSeverityVariant ? alertSeverityScreenReaderText || `${alertSeverityVariant} alert:` : undefined


instead of undefined, can we just default it to ${alertSeverityVariant} alert:?

tlabaj

LGTM

mcoker

🥳

patternfly-build · 2021-12-09T15:59:47Z

Your changes have been released in:

eslint-plugin-patternfly-react@4.20.0
@patternfly/react-catalog-view-extension@4.32.0
@patternfly/react-charts@6.34.0
@patternfly/react-code-editor@4.22.0
@patternfly/react-console@4.32.0
@patternfly/react-core@4.181.0
@patternfly/react-docs@5.42.0
@patternfly/react-icons@4.32.0
@patternfly/react-inline-edit-extension@4.26.0
demo-app-ts@4.141.0
@patternfly/react-integration@4.143.0
@patternfly/react-log-viewer@4.26.0
@patternfly/react-styles@4.31.0
@patternfly/react-table@4.50.0
@patternfly/react-tokens@4.33.0
@patternfly/react-topology@4.28.0
@patternfly/react-virtualized-extension@4.28.0
transformer-cjs-imports@4.19.0

Thanks for your contribution! 🎉

tlabaj requested review from jschuler, mcarrano and mcoker December 2, 2021 16:29

tlabaj added ux review labels Dec 2, 2021

mcarrano approved these changes Dec 2, 2021

View reviewed changes

nicolethoen approved these changes Dec 2, 2021

View reviewed changes

mcoker requested changes Dec 2, 2021

View reviewed changes

Comment thread packages/react-core/src/components/Popover/examples/Popover.md Outdated

Comment thread packages/react-core/src/components/Popover/examples/Popover.md Outdated

jschuler reviewed Dec 3, 2021

View reviewed changes

nicolethoen force-pushed the popover-alert branch from edcc4e5 to 1778593 Compare December 7, 2021 15:44

nicolethoen requested a review from tlabaj December 7, 2021 15:59

tlabaj reviewed Dec 7, 2021

View reviewed changes

wise-king-sullyman reviewed Dec 7, 2021

View reviewed changes

wise-king-sullyman approved these changes Dec 8, 2021

View reviewed changes

tlabaj approved these changes Dec 8, 2021

View reviewed changes

mcoker reviewed Dec 8, 2021

View reviewed changes

tlabaj reviewed Dec 8, 2021

View reviewed changes

tlabaj requested changes Dec 8, 2021

View reviewed changes

kmcfaul and others added 7 commits December 8, 2021 16:00

feat(Popover): add composable header and alert variants

b5e580a

add icon example

c6e6d14

update sr for icon example

dc42fa1

fix docs build

978da42

Apply suggestions from code review

a533c66

adding modifier for icon Co-authored-by: Michael Coker <35148959+mcoker@users.noreply.github.com>

add className where applicable

f4724c4

feat(Popover): add composable header and alert variants

1b7f099

kmcfaul and others added 12 commits December 8, 2021 16:00

add icon example

a1b4d84

update sr for icon example

bb3e317

fix docs build

c06b292

Apply suggestions from code review

527c078

adding modifier for icon Co-authored-by: Michael Coker <35148959+mcoker@users.noreply.github.com>

remove duplicate examples

d631215

change alertVariant to alertSeverityVariant

b5928f5

mark new examples beta

90301fd

Remove unused id prop

9166014

back out composable solution for something more intuitive

464f81f

delete internal header

67967af

dont export header components

5a9cdfd

make screen reader text configurable

aadb573

mcoker requested changes Dec 8, 2021

View reviewed changes

clean up

f979a64

nicolethoen force-pushed the popover-alert branch from 0d205c2 to f979a64 Compare December 8, 2021 21:28

tlabaj reviewed Dec 8, 2021

View reviewed changes

update default screen reader text

2b911a0

tlabaj approved these changes Dec 9, 2021

View reviewed changes

update snapshots

7dfbdd9

mcoker approved these changes Dec 9, 2021

View reviewed changes

tlabaj merged commit 9028e8b into patternfly:main Dec 9, 2021

tlabaj mentioned this pull request Dec 9, 2021

Banner examples need screen reader text/label indicating the context #5890

Closed

nicolethoen mentioned this pull request Dec 9, 2021

Notification read/unread state should be indicated to screen reader in notification drawer #5889

Closed

Conversation

kmcfaul commented Dec 1, 2021 • edited by nicolethoen Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patternfly-build commented Dec 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcarrano left a comment

Choose a reason for hiding this comment

Uh oh!

nicolethoen left a comment

Choose a reason for hiding this comment

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!

jschuler Dec 7, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

tlabaj 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!

Choose a reason for hiding this comment

Uh oh!

tlabaj Dec 8, 2021 • 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!

tlabaj Dec 8, 2021 • 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!

tlabaj Dec 8, 2021 • 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!

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!

tlabaj left a comment

Choose a reason for hiding this comment

Uh oh!

mcoker left a comment

Choose a reason for hiding this comment

Uh oh!

patternfly-build commented Dec 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

kmcfaul commented Dec 1, 2021 •

edited by nicolethoen

Loading

patternfly-build commented Dec 1, 2021 •

edited

Loading

jschuler Dec 7, 2021 •

edited

Loading

tlabaj Dec 8, 2021 •

edited

Loading

tlabaj Dec 8, 2021 •

edited

Loading

tlabaj Dec 8, 2021 •

edited

Loading