feat(notification-drawer): added notification drawer component #2511

mcoker · 2019-12-11T20:15:18Z

Given the number of overrides that would be needed to fit the accordion to this component, I opted just to create a unique element in this component. That may change in the future.

The notification drawer should fill the height of its container.
Unread notifications should be clickable to mark them as read.
Expanded groups should push all of the other groups underneath it down until 1) all of the items in the group are visible, or 2) the groups underneath hit the bottom of the container at which point the list of notifications should scroll within the group. A couple of visuals showing this:

Expanded groups have a min-height of 300px, so if the viewport shrinks in height and there isn't enough space to display the expanded group and all of the additional groups, an outer scrollbar will appear, creating 2 scrollbars and a scroll treadmill trap. This isn't ideal, but it should be a rare occurrence as there will likely only be a few groups in a notification drawer. We will likely iterate more on this interaction. Here's a screengrab of that.

I deviated from the __el-nested-nested BEM naming in a couple of places because the classnames were getting really long and it didn't seem necessary to follow that structure re: avoiding class name conflicts in the future. I'm open to changing this.

patternfly-build · 2019-12-11T21:20:11Z

PatternFly-Next preview: https://patternfly-next-pr-2511.surge.sh

mcarrano · 2019-12-12T13:53:57Z

@mcoker This is looking much better following recent changes. I was comparing this to the design doc here: https://docs.google.com/document/d/1kccmdwXgnkLZdH5OY_sr6C91Gsm0gCtF6zp9OBwC-dE/edit?usp=sharing One thing that is missing is that the unread item should be clickable as we want the user to be able to mark as read with a single click and not need to open the kabob.

@maryshak1996 can you also take a look at this? One thing I'm wondering is whether we should have hover states for elements that are clickable, like an unread notification item. Seems like the group headers should also have a hover as they do in the Accordion component. In fact, should we just be reusing the Accordion here @mcoker ?

mcoker · 2019-12-12T17:02:52Z

@mcarrano updated the PR so that the unread items can receive keyboard focus and have a cursor pointer to indicate they're interactive.

In fact, should we just be reusing the Accordion here

I suppose it depends on how far the group list and accordion component end up diverging. At the moment there would be quite a bit of overrides to the component to use it in the notification drawer, then we create a dependency between those 2 components. I would say that with what we have currently, the styling differences are enough that I think we can create this as a standalone panel in the notification drawer.

christiemolloy · 2019-12-13T15:22:02Z

@mcarrano @maryshak1996 if the title wraps like this should the paragraph of text underneath it be tabbed over more?

christiemolloy · 2019-12-13T15:52:09Z

src/patternfly/components/NotificationDrawer/examples/NotificationDrawer.md

+| `.pf-c-notification-drawer__list-item-header-title` | `<h2>` | Initiates a notification list item header title. |
+| `.pf-c-notification-drawer__list-item-action` | `<div>` | Initiates a notification list item action. |
+| `.pf-c-notification-drawer__list-item-description` | `<div>` | Initiates a notification list item description. |
+| `.pf-c-notification-drawer__list-item-timestamp` | `<div>` | Initiates a notification list item timestamp. |


will this always be a timestamp, wondering if this class name is too specific ?

Agree it's specific. This could be a general __list-item-footer element, wdyt?

christiemolloy · 2019-12-13T15:56:11Z

src/patternfly/components/NotificationDrawer/notification-drawer-header-title.hbs

@@ -0,0 +1,6 @@
+<h1 class="pf-c-notification-drawer__header-title{{#if notification-drawer-header-title--modifier}} {{notification-drawer-header-title--modifier}}{{/if}}"


Should the h1 tag be on the title element?

Where do you think it could go instead?

christiemolloy · 2019-12-13T16:09:54Z

src/patternfly/components/NotificationDrawer/examples/NotificationDrawer.md

+| `.pf-c-notification-drawer__group-title` | `<div>` | Initiates a notification group toggle title. |
+| `.pf-c-notification-drawer__group-count` | `<div>` | Initiates a notification group toggle count. |
+| `.pf-c-notification-drawer__group-icon` | `<span>` | Initiates a notification group toggle icon. |
+| `.pf-m-info` | `.pf-c-notification-drawer__list-item` | Modifies a notification list item for the info state. |


Is it necessary to have a modifier class on the list-item?

Yep to indicate what kind of notification it is.

christiemolloy · 2019-12-13T16:10:16Z

src/patternfly/components/NotificationDrawer/examples/NotificationDrawer.md

+| `.pf-m-danger` | `.pf-c-notification-drawer__list-item` | Modifies a notification list item for the danger state. |
+| `.pf-m-success` | `.pf-c-notification-drawer__list-item` | Modifies a notification list item for the success state. |
+| `.pf-m-read` | `.pf-c-notification-drawer__list-item` | Modifies a notification list item for the read state. |
+| `.pf-m-clickable` | `.pf-c-notification-drawer__list-item` | Modifies a notification list item to indicate that it is clickable. |


should this be pf-m-selectable instead?

I wondered that, and if we might have a "selectable" state in the future that might be separate from what we're offering here, which is to indicate that the item is clickable. The way I envision it currently in the react component is the item can attach click events to it like, on click, add .pf-m-read, or execute a custom callback function. That's what this class adds are styles to say "you can click this and something will happen."

And I wonder if something could be both selectable and clickable, with different styles. Maybe pf-m-hoverable? We use that in other places to indicate hover styles to say something is interactive.

christiemolloy · 2019-12-13T16:18:14Z

src/patternfly/components/NotificationDrawer/notification-drawer.scss

+  --pf-c-notification-drawer__group-toggle-icon--Transition: .2s ease-in 0s;
+  --pf-c-notification-drawer__group--m-expanded__group-toggle-icon--Transform: rotate(90deg);
+
+  display: flex;


Is this necessary?

Yeah, so that the __header will stay fixed and __body will be the scrollable item if the viewport height is shorter than the overall list of things in the notification drawer. Or do you think we can achieve that without it?

mcarrano

Looks great. Thanks for adding the hover states @mcoker !

mattnolting

Nice work! Couple of comments.. But overall LGTM!

mattnolting · 2019-12-13T20:23:24Z

src/patternfly/components/NotificationDrawer/notification-drawer-basic-list.hbs

+      {{/notification-drawer-list-item-header-title}}
+    {{/notification-drawer-list-item-header}}
+    {{#> notification-drawer-list-item-action}}
+      {{#> dropdown id=(concat notification-drawer--id "-action1") dropdown--IsActionMenu="true" dropdown-toggle--modifier="pf-m-plain" dropdown--HasKebabIcon="true" aria-label="Actions"}}{{/dropdown}}


Need to bump {{#> dropdown id=(concat notification-drawer--id "-action1")... to -action4

mattnolting · 2019-12-13T21:06:46Z

src/patternfly/components/NotificationDrawer/notification-drawer.scss

+
+    &:hover {
+      z-index: 100;
+      box-shadow: var(--pf-global--BoxShadow--md-top), var(--pf-global--BoxShadow--md-bottom);


Should these be --pf-c-notification-drawer__list-item--m-hoverable--BoxShadow and --pf-c-notification-drawer__list-item--m-hoverable--ZIndex

redallen · 2019-12-13T22:19:08Z

🎉 This PR is included in version 2.46.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

mcoker requested a review from mcarrano December 11, 2019 20:15

mcoker force-pushed the issue-1787 branch 2 times, most recently from 4db9f0a to cfccd22 Compare December 11, 2019 22:59

mcoker closed this Dec 12, 2019

mcoker force-pushed the issue-1787 branch from de2c7cd to 6177911 Compare December 12, 2019 19:26

mcoker reopened this Dec 12, 2019

mcoker requested review from mattnolting and christiemolloy December 12, 2019 21:05

mcoker force-pushed the issue-1787 branch from af6c688 to b43cbcc Compare December 12, 2019 21:25

feat(notification-drawer): added notification drawer component

beea45c

mcoker force-pushed the issue-1787 branch from b43cbcc to beea45c Compare December 12, 2019 21:32

fix(notification-drawer): add icon transition

bf5b75e

mcoker added 2 commits December 13, 2019 09:30

fix(notification-drawer): header stay fixed when groups scroll

cf2ef74

fix(notification-drawer): docs and add clickable variation

5bd8f0d

christiemolloy requested changes Dec 13, 2019

View reviewed changes

mcoker added 3 commits December 13, 2019 10:43

fix(notification-drawer): clickable to hoverable

50d5277

fix(notification-drawer): ability to remove list-item-action

d1d9c42

fix(notification-drawer): add group list, update docs

9bff60c

mcarrano approved these changes Dec 13, 2019

View reviewed changes

mattnolting approved these changes Dec 13, 2019

View reviewed changes

mcoker added 3 commits December 13, 2019 15:30

fix(notification-drawer): a11y

3f22834

fix(notification-drawer): update vars

dccf78e

fix(notification-drawer): align list item header baseline

2ca1f28

christiemolloy merged commit 0bdfa8a into patternfly:master Dec 13, 2019

redallen added the released label Dec 13, 2019

mcoker deleted the issue-1787 branch December 16, 2019 22:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(notification-drawer): added notification drawer component #2511

feat(notification-drawer): added notification drawer component #2511

mcoker commented Dec 11, 2019 •

edited

Loading

patternfly-build commented Dec 11, 2019

mcarrano commented Dec 12, 2019

mcoker commented Dec 12, 2019

christiemolloy commented Dec 13, 2019

christiemolloy Dec 13, 2019

mcoker Dec 13, 2019

christiemolloy Dec 13, 2019

mcoker Dec 13, 2019

christiemolloy Dec 13, 2019

mcoker Dec 13, 2019

christiemolloy Dec 13, 2019

mcoker Dec 13, 2019

christiemolloy Dec 13, 2019

mcoker Dec 13, 2019

mcarrano left a comment

mattnolting left a comment

mattnolting Dec 13, 2019

mattnolting Dec 13, 2019

redallen commented Dec 13, 2019

		@@ -0,0 +1,6 @@
		<h1 class="pf-c-notification-drawer__header-title{{#if notification-drawer-header-title--modifier}} {{notification-drawer-header-title--modifier}}{{/if}}"

feat(notification-drawer): added notification drawer component #2511

feat(notification-drawer): added notification drawer component #2511

Conversation

mcoker commented Dec 11, 2019 • edited Loading

patternfly-build commented Dec 11, 2019

mcarrano commented Dec 12, 2019

mcoker commented Dec 12, 2019

christiemolloy commented Dec 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcarrano left a comment

Choose a reason for hiding this comment

mattnolting left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redallen commented Dec 13, 2019

mcoker commented Dec 11, 2019 •

edited

Loading