feat(NotificationDrawer): Add notification drawer demo #4640

nicolethoen · 2020-07-31T18:30:31Z

What: Closes #4535

Additional issues:

patternfly-build · 2020-07-31T18:42:33Z

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

mcoker · 2020-07-31T19:51:24Z

@nicolethoen this looks great! A few things:

The parent .pf-c-page__header-tools-item element with the notification bell icon should have .pf-m-selected when the drawer is open.
The notification bell disappears on a mobile viewport. It should be visible on all viewports
The grouped notification drawer is showing some old styles - namely that groups shouldn't scroll independently of the overall notification list. You can compare this demo to the core grouped demo. I assume that will resolve with a core bump?

mcarrano

This looks great @nicolethoen . I have the same comment about scrolling behavior as @mcoker . The only other thing is that this still uses the old notification badge component. Any reason we wouldn't pull the new component into this demo?

nicolethoen · 2020-08-03T13:53:23Z

This looks great @nicolethoen . I have the same comment about scrolling behavior as @mcoker . The only other thing is that this still uses the old notification badge component. Any reason we wouldn't pull the new component into this demo?

@mcarrano
the new react notification badge has not been merged yet. It should update automatically.

Once merged, is there any reason one of these demos should use the 'attention state' notification badge?

mcarrano · 2020-08-03T16:47:23Z

@nicolethoen regarding the attention state, yes, it would make sense to have a demo of that. However I'm not sure that we totally defined all the rules for when this appear/disappears so maybe pick that up in a follow-on issue?

nicolethoen · 2020-08-03T16:59:52Z

@nicolethoen regarding the attention state, yes, it would make sense to have a demo of that. However I'm not sure that we totally defined all the rules for when this appear/disappears so maybe pick that up in a follow-on issue?

@mcarrano
should I just use the attention state in one of the existing two examples on this PR or make a whole new example?

kmcfaul

Looks good to me aside from the double scrolling on groups, which should be updated in a that core bump if I'm understanding correctly.

mcarrano · 2020-08-03T21:20:28Z

@mceledonia Can you take a look at the behavior of the notification badge and verify that it is correct. Should the 'unread' state only display when the drawer is closed? Other than that, I think this is good to go @nicolethoen . It might be nice to show an example of the notification count in the badge, we we could always add that later.

nicolethoen · 2020-08-03T21:25:58Z

@mceledonia Can you take a look at the behavior of the notification badge and verify that it is correct. Should the 'unread' state only display when the drawer is closed? Other than that, I think this is good to go @nicolethoen . It might be nice to show an example of the notification count in the badge, we we could always add that later.

I think that is a result of the pf-m-selected modifier being applied to the badge when the drawer is open.

codecov-commenter · 2020-08-03T22:01:44Z

Codecov Report

❗ No coverage uploaded for pull request base (master@cc17bf2). Click here to learn what that means.
The diff coverage is 71.42%.

@@            Coverage Diff            @@
##             master    #4640   +/-   ##
=========================================
  Coverage          ?   52.41%           
=========================================
  Files             ?      509           
  Lines             ?     8944           
  Branches          ?     3253           
=========================================
  Hits              ?     4688           
  Misses            ?     3670           
  Partials          ?      586

Flag	Coverage Δ
#patternfly4	`52.41% <71.42%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
packages/react-core/src/components/Page/Page.tsx	`73.17% <66.66%> (ø)`
...ts/NotificationDrawer/NotificationDrawerHeader.tsx	`100.00% <100.00%> (ø)`
...ckages/react-core/src/components/Banner/Banner.tsx	`100.00% <0.00%> (ø)`
...react-core/src/components/Modal/ModalBoxHeader.tsx	`80.00% <0.00%> (ø)`
packages/react-topology/src/elements/BaseEdge.ts	`1.61% <0.00%> (ø)`
...ion/src/components/ConfirmButton/ConfirmButton.tsx	`100.00% <0.00%> (ø)`
...tension/src/components/CatalogTile/CatalogTile.tsx	`80.00% <0.00%> (ø)`
...-core/src/components/Page/PageHeaderToolsGroup.tsx	`100.00% <0.00%> (ø)`
...core/src/components/DataList/DataListItemCells.tsx	`100.00% <0.00%> (ø)`
...ChartBullet/ChartBulletComparativeErrorMeasure.tsx	`92.30% <0.00%> (ø)`
... and 501 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 cc17bf2...5b7506b. Read the comment docs.

jschuler · 2020-08-04T17:04:10Z

packages/react-core/src/components/NotificationDrawer/NotificationDrawerHeader.tsx

    </Text>
-    {count && <span className={css(styles.notificationDrawerHeaderStatus)}>{`${count} ${unreadText}`}</span>}
+    {count !== undefined && (
+      <span className={css(styles.notificationDrawerHeaderStatus)}>{`${count} ${unreadText}`}</span>


this seems a bit too rigid of a format and won't work well with i18n

What do you propose?

Not a blocker if this was already released, but I question if the count prop here is needed, user could just pass <NotificationDrawerHeader unreadText=`${count} unread` />

ah
NotificationDrawerGroup also expects a count: number; to display in its header.

I guess I'm not sure I see a big difference, and it would be a breaking change to remove count from NotificaitonDrawerHeader

It's a little different in the drawer group because it's just the count there. Here we mandate that count is followed by a space and then a string which is not correct for some languages. This already existed before so not a blocker for this PR tho! But we are accruing technical debt here, since this is a beta component I am not sure if breaking changes are allowed or not.

mturley

Looks good to me! One possible enhancement that we could maybe add as a followup PR: since you have a "clear all" action, should we maybe put "clear" actions on each notification too? I don't see a way to clear just one, it might be a nice thing to demonstrate.

patternfly-build · 2020-08-04T20:06:00Z

Your changes have been released in:

@patternfly/react-catalog-view-extension@4.8.0
@patternfly/react-charts@6.9.0
@patternfly/react-core@4.39.0
@patternfly/react-docs@5.8.0
@patternfly/react-icons@4.7.0
@patternfly/react-inline-edit-extension@4.5.56
demo-app-ts@4.28.2
@patternfly/react-styles@4.7.0
@patternfly/react-table@4.15.0
@patternfly/react-tokens@4.9.0
@patternfly/react-topology@4.4.58
@patternfly/react-virtualized-extension@4.5.46

Thanks for your contribution! 🎉

nicolethoen requested review from jenny-s51, kmcfaul, mcarrano, mcoker and tlabaj July 31, 2020 18:30

mcarrano reviewed Jul 31, 2020

View reviewed changes

tlabaj added ux review labels Aug 3, 2020

kmcfaul previously approved these changes Aug 3, 2020

View reviewed changes

nicolethoen dismissed kmcfaul’s stale review via 1904bce August 3, 2020 20:57

nicolethoen force-pushed the notification_drawer_demo branch from 44428cb to 1904bce Compare August 3, 2020 20:57

nicolethoen added 6 commits August 4, 2020 11:40

feat(NotificationDrawer): Add notificaiton drawer demo

e3745e7

fix snapshots

c5fd48a

reorgainize PageHeaderToolItems in demos

abc9496

address PR comments after incorporating updated notification badge

2a06062

only apply selected modifier if notifications all read

33daf88

bump version and fix selected icon modifier application

42c7fd1

nicolethoen force-pushed the notification_drawer_demo branch from 14cac83 to 42c7fd1 Compare August 4, 2020 15:42

jschuler reviewed Aug 4, 2020

View reviewed changes

jschuler approved these changes Aug 4, 2020

View reviewed changes

kmcfaul approved these changes Aug 4, 2020

View reviewed changes

mcoker approved these changes Aug 4, 2020

View reviewed changes

mturley approved these changes Aug 4, 2020

View reviewed changes

tlabaj merged commit 3b9231d into patternfly:master Aug 4, 2020

nicolethoen deleted the notification_drawer_demo branch February 8, 2023 13:53

feat(NotificationDrawer): Add notification drawer demo #4640

feat(NotificationDrawer): Add notification drawer demo #4640

Uh oh!

Conversation

nicolethoen commented Jul 31, 2020

Uh oh!

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

Uh oh!

Uh oh!

mcoker commented Jul 31, 2020

Uh oh!

mcarrano left a comment

Choose a reason for hiding this comment

Uh oh!

nicolethoen commented Aug 3, 2020

Uh oh!

mcarrano commented Aug 3, 2020

Uh oh!

nicolethoen commented Aug 3, 2020

Uh oh!

kmcfaul left a comment

Choose a reason for hiding this comment

Uh oh!

mcarrano commented Aug 3, 2020

Uh oh!

nicolethoen commented Aug 3, 2020

Uh oh!

codecov-commenter commented Aug 3, 2020

Codecov Report

Uh oh!

jschuler Aug 4, 2020

Choose a reason for hiding this comment

Uh oh!

nicolethoen Aug 4, 2020

Choose a reason for hiding this comment

Uh oh!

jschuler Aug 4, 2020 • 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 Aug 4, 2020

Choose a reason for hiding this comment

Uh oh!

jschuler Aug 4, 2020

Choose a reason for hiding this comment

Uh oh!

jschuler Aug 4, 2020

Choose a reason for hiding this comment

Uh oh!

mturley left a comment

Choose a reason for hiding this comment

Uh oh!

patternfly-build commented Aug 4, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

patternfly-build commented Jul 31, 2020 •

edited

Loading

jschuler Aug 4, 2020 •

edited

Loading