Feat(NotificationDrawer): Add notification demo - #3220

Preview: https://patternfly-pr-3220.surge.sh

A11y report: https://patternfly-pr-3220-coverage.surge.sh

@jcaianirh from what I know I think having the entire notification drawer scroll when there are groups is an override to what we offer in PatternFly, but ill defer to @mcoker and @mcarrano because that might be a variation that we can offer.

@mcarrano @mcoker I wanted to experiment with having two groups open at once to make sure double scrollbars dont appear. Is this expected in the notification drawer, or is it the case that as soon as you expand a group, the other groups close?

@christiemolloy the way this is implemented now matches the design intent. @jcaianirh is there a reason you would not want it to work this way? I would not be in favor of having multiple examples. We should just agree on the way that this works and have a consistent approach. As to the correct accordion behavior, looks like the way it is implemented in React, each group opens independently (https://www.patternfly.org/v4/documentation/react/components/notificationdrawer#groups). I'm fine wit that.

Should the demo also include an example of the basic drawer without groups?

@mcarrano that sounds good to me. I'll add that as a second example for the demo

@mcoker @mceledonia interested to hear your thoughts on the scrolling issue.

@christiemolloy after chatting with @jcaianirh @mcoker and @mceledonia , we decided that the default behavior should be as @jcaianirh describes - that each group opens independently and a scroll bar is exposed for the entire contents of the drawer rather than constraining the height of the group as you currently have it. So this demo should then include two versions of the drawer. One with groups and the basic drawer without groups. Thanks!

@mcarrano @jcaianirh @mcoker updated the examples so now there is both variations, and the notification drawer's default behavior has changed

@christiemolloy So, the contents of the notification drawer body are supposed to scroll, but not the header. The header remains in place while the body contents scroll vertically. Thanks for updating the behavior.

Ah thanks for catching that! will update shortly

@jcaianirh do you mind taking a look at it now ?

• We currently have a Drawer demo and a Notification Drawer demo. Any reason we need both? THey seem like the same thing.

@mcarrano good point looks the same to me

• @mattnolting is currently working on updating the Notification Badge in the masthead. Will that change just get picked up here? I assume so, but wanted to make sure you are aware. If needed you guys should coordinate.

@mcarrano i would think if this is gets picked up here it would still need modification since the new badge has more info to offer than just a boolean on/off...there are counts and severities needed to populate it with corresponding data. So @christiemolloy this will probably need to be updated when the new badge is delivered.

@mcarrano @jcaianirh

• Updated the examples to say 2 unread instead of 3 unread
• We have a Drawer demo and Notification demo because the drawer is a separate component where any kind of list could go into the Drawer vs the Notification demo is its own demo, but let me know if you think otherwise.
• Ill coordinate with @mattnolting on updating this demo once the badge is complete!

We have a Drawer demo and Notification demo because the drawer is a separate component where any kind of list could go into the Drawer vs the Notification demo is its own demo, but let me know if you think otherwise.

I understand what you are saying, but it just seems odd to have two versions of the same thing. Maybe we just go ahead and merge this then and we can create something different for the Drawer demo at a later date. Or do we even need a Drawer demo on it's own? Thoughts @mcoker ?

I will go ahead and give this one more review and approve.

@mcarrano the drawer demo was to create it in context of the page component for the use with notification drawer, but we didn't have the notification drawer built yet. Now that we have the notification drawer demo, I think having the two doesn't seem necessary as they are, since the "drawer" demo is just an empty drawer.

we can create something different for the Drawer demo at a later date

I'm in favor of that. Maybe showing how to use a simple list in a drawer in a page section, or other uses of the drawer. If it makes sense to you, we could go ahead and move this demo the the "drawer" demo page, and just have this as a drawer demo called "notification drawer."

If it makes sense to you, we could go ahead and move this demo the the "drawer" demo page, and just have this as a drawer demo called "notification drawer."

@mcoker I'd rather not do this because the notification drawer is such a simple thing and I think people will look for it under that name in the navigation. Let's leave the drawer demo as is for now (just an empty drawer). I've opened a new issue to design a more robust drawer demo in the future that can include additional features like the horizontal drawer and splitter that are planned. patternfly/patternfly-design#891

I wanted to get rid of the .pf-m-overflow-hidden class and was able to do so, but this exposed a bug @jcaianirh and I discovered earlier in the year where there would be a lot of unnecessary white space at the bottom of the drawer, causing a double scrollbar. After troubleshooting that (for way too long), I tracked it down to position: fixed on the .pf-screen-reader text in the notification alert item title text inside of the drawer panel that has translateX(). This is a chrome specific issue (FF works just fine), and I created a simple demo outlining the problem here - https://codepen.io/mcoker/pen/eYJxbzb?editors=1100

I've opened a PR on @christiemolloy's branch with some updates that enable dropping .pf-m-overflow-hidden and clean up some styles left over from when the notification drawer was more flexy in its overflow behavior.

Let's continue to discuss a solution to the screen reader text issue.

@christiemolloy noticed a bug in safari where height: 100% on the notification drawer isn't working due to .pf-c-drawer__body:last-child {flex: 1 1 auto; } - the auto flex basis isn't giving it an implicit height, so height: 100% won't work.

Just changing that to flex: 1 1; will fix it - Safari will assign flex-basis: 0%; in the absence of it declared int he shorthand value.