-
Notifications
You must be signed in to change notification settings - Fork 95
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(notification-drawer): added notification drawer component #2511
Conversation
PatternFly-Next preview: https://patternfly-next-pr-2511.surge.sh |
4db9f0a
to
cfccd22
Compare
@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 ? |
@mcarrano updated the PR so that the unread items can receive keyboard focus and have a cursor pointer to indicate they're interactive.
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. |
@mcarrano @maryshak1996 if the title wraps like this should the paragraph of text underneath it be tabbed over more? |
| `.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. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will this always be a timestamp, wondering if this class name is too specific ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree it's specific. This could be a general __list-item-footer
element, wdyt?
@@ -0,0 +1,6 @@ | |||
<h1 class="pf-c-notification-drawer__header-title{{#if notification-drawer-header-title--modifier}} {{notification-drawer-header-title--modifier}}{{/if}}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the h1 tag be on the title element?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where do you think it could go instead?
| `.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. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to have a modifier class on the list-item?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep to indicate what kind of notification it is.
| `.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. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be pf-m-selectable
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
--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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. Thanks for adding the hover states @mcoker !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! Couple of comments.. But overall LGTM!
{{/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}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to bump {{#> dropdown id=(concat notification-drawer--id "-action1")...
to -action4
|
||
&:hover { | ||
z-index: 100; | ||
box-shadow: var(--pf-global--BoxShadow--md-top), var(--pf-global--BoxShadow--md-bottom); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these be --pf-c-notification-drawer__list-item--m-hoverable--BoxShadow
and --pf-c-notification-drawer__list-item--m-hoverable--ZIndex
🎉 This PR is included in version 2.46.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
fixes #1787
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.
__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.