Implement Notification Drawer Updates #675
Conversation
jeff-phillips-18
force-pushed
the
notifications
branch
from
bb1e7de to
bb216e8
Compare
|
Thanks @jeff-phillips-18 the badge, mark as read, clear, and empty state are working great. A few things I noticed were (1) in the expanded state, the close x and the title are not in the correct places. The drawer title should be centered and the close should still remain in the far right corner. (2) When clicking on an individual cell that is "new" (bold font) it should become marked as read and unbolded. |
|
@beanh66 I will take a look at the close button issue. The Mark read was not implemented before this PR. I can add the JS to make it so. |
jeff-phillips-18
force-pushed
the
notifications
branch
from
bb216e8 to
984ce6a
Compare
|
@beanh66 Updated to address both issues. |
|
The implementation looks great. It seems that @beanh66 's comments were addressed, as well! |
|
@jeff-phillips-18 thanks the centering looks great now. Just wondering about the cursor change for the clicking to mark as read. Changing to the text editor cursor makes it seem like the rows are not clickable. Should that stay as a pointer or change to the click pointer? It is a weird case since it doesn't actually link to anything, but IMO it should still look clickable. Separate nit, can the expand arrow change to the blue on hover since we do that for expanding list view items as well? |
jeff-phillips-18
force-pushed
the
notifications
branch
from
984ce6a to
55734d0
Compare
|
@beanh66 Changed to the default pointer cursor and added the link color on hover of the drawer expander. |
|
Thanks, LGTM! |
Implementation of patternfly/patternfly-design#297
jeff-phillips-18
force-pushed
the
notifications
branch
from
55734d0 to
42712d4
Compare
|
@beanh66 @andresgalante @bleathem Updated removing changes for the unread notifications indicator which were done in #678. Can you please please review this please :) |
| <div class="drawer-pf hide drawer-pf-notifications-non-clickable"> | ||
| <div class="drawer-pf-title"> | ||
| <a class="drawer-pf-toggle-expand"></a> | ||
| <a class="drawer-pf-close"></a> |
There was a problem hiding this comment.
Is there a reason why we are using generated icon with css instead of adding it on the html?
There was a problem hiding this comment.
Using pficon-close sets the line height to 1, and we would need to re-override it back to 1.66667 to align correctly. I can make that change if desired.
There was a problem hiding this comment.
That sucks :( Since the icons on the masthead are not generated I think we should do the same here. I'd go with a real icon on the html.
|
@jeff-phillips-18 Other than the generated icons I think everything is ok. Thanks a lot Jeff! |
|
This looks good to me, nice work Jeff! |
|
👍 Thanks a lot @jeff-phillips-18 I am ok to merge this. @bleathem when you can please check the JS |
|
The js looks fine to me. Thanks for the PR @jeff-phillips-18! |
6 participants
Implementation of
patternfly/patternfly-design#297
Description
This PR add implementations for the notification drawer updates:
View and test at:
@beanh66 @serenamarie125 @jennyhaines