From 3ca035f9aad71101ffc80b9cd8ff9d3ad2368e68 Mon Sep 17 00:00:00 2001
From: David Wheatley <hi@davwheat.dev>
Date: Sun, 2 May 2021 17:13:04 +0100
Subject: [PATCH] Revamp notifications stylesheet (grid and flex) (#2822)

---
 js/src/forum/components/Notification.js       |  31 ++-
 js/src/forum/components/NotificationList.js   |  24 +-
 .../forum/components/NotificationsDropdown.js |  10 +-
 less/forum/NotificationList.less              | 244 +++++++++++-------
 4 files changed, 186 insertions(+), 123 deletions(-)

diff --git a/js/src/forum/components/Notification.js b/js/src/forum/components/Notification.js
index e9ce7092c..32ba8bb6c 100644
--- a/js/src/forum/components/Notification.js
+++ b/js/src/forum/components/Notification.js
@@ -4,6 +4,7 @@ import icon from '../../common/helpers/icon';
 import humanTime from '../../common/helpers/humanTime';
 import Button from '../../common/components/Button';
 import Link from '../../common/components/Link';
+import classList from '../../common/utils/classList';
 
 /**
  * The `Notification` component abstract displays a single notification.
@@ -22,27 +23,31 @@ export default class Notification extends Component {
 
     return (
       <Link
-        className={'Notification Notification--' + notification.contentType() + ' ' + (!notification.isRead() ? 'unread' : '')}
+        className={classList('Notification', `Notification--${notification.contentType()}`, [!notification.isRead() && 'unread'])}
         href={href}
         external={href.includes('://')}
         onclick={this.markAsRead.bind(this)}
       >
-        {!notification.isRead() &&
-          Button.component({
-            className: 'Notification-action Button Button--icon Button--link',
-            icon: 'fas fa-check',
-            title: app.translator.trans('core.forum.notifications.mark_as_read_tooltip'),
-            onclick: (e) => {
+        {avatar(notification.fromUser())}
+        {icon(this.icon(), { className: 'Notification-icon' })}
+        <span className="Notification-title">
+          <span className="Notification-content">{this.content()}</span>
+          <span className="Notification-title-spring" />
+          {humanTime(notification.createdAt())}
+        </span>
+        {!notification.isRead() && (
+          <Button
+            className="Notification-action Button Button--link"
+            icon="fas fa-check"
+            title={app.translator.trans('core.forum.notifications.mark_as_read_tooltip')}
+            onclick={(e) => {
               e.preventDefault();
               e.stopPropagation();
 
               this.markAsRead();
-            },
-          })}
-        {avatar(notification.fromUser())}
-        {icon(this.icon(), { className: 'Notification-icon' })}
-        <span className="Notification-content">{this.content()}</span>
-        {humanTime(notification.createdAt())}
+            }}
+          />
+        )}
         <div className="Notification-excerpt">{this.excerpt()}</div>
       </Link>
     );
diff --git a/js/src/forum/components/NotificationList.js b/js/src/forum/components/NotificationList.js
index ab7a62d71..74950501b 100644
--- a/js/src/forum/components/NotificationList.js
+++ b/js/src/forum/components/NotificationList.js
@@ -17,16 +17,16 @@ export default class NotificationList extends Component {
     return (
       <div className="NotificationList">
         <div className="NotificationList-header">
-          <div className="App-primaryControl">
-            {Button.component({
-              className: 'Button Button--icon Button--link',
-              icon: 'fas fa-check',
-              title: app.translator.trans('core.forum.notifications.mark_all_as_read_tooltip'),
-              onclick: state.markAllAsRead.bind(state),
-            })}
-          </div>
-
           <h4 className="App-titleControl App-titleControl--text">{app.translator.trans('core.forum.notifications.title')}</h4>
+
+          <div className="App-primaryControl">
+            <Button
+              className="Button Button--link"
+              icon="fas fa-check"
+              title={app.translator.trans('core.forum.notifications.mark_all_as_read_tooltip')}
+              onclick={state.markAllAsRead.bind(state)}
+            />
+          </div>
         </div>
 
         <div className="NotificationList-content">
@@ -43,7 +43,7 @@ export default class NotificationList extends Component {
                   // Get the discussion that this notification is related to. If it's not
                   // directly related to a discussion, it may be related to a post or
                   // other entity which is related to a discussion.
-                  let discussion = false;
+                  let discussion = null;
                   if (subject instanceof Discussion) discussion = subject;
                   else if (subject && subject.discussion) discussion = subject.discussion();
 
@@ -65,8 +65,8 @@ export default class NotificationList extends Component {
                     <div className="NotificationGroup">
                       {group.discussion ? (
                         <Link className="NotificationGroup-header" href={app.route.discussion(group.discussion)}>
-                          {badges && badges.length ? <ul className="NotificationGroup-badges badges">{listItems(badges)}</ul> : ''}
-                          {group.discussion.title()}
+                          {badges && badges.length && <ul className="NotificationGroup-badges badges">{listItems(badges)}</ul>}
+                          <span>{group.discussion.title()}</span>
                         </Link>
                       ) : (
                         <div className="NotificationGroup-header">{app.forum.attribute('title')}</div>
diff --git a/js/src/forum/components/NotificationsDropdown.js b/js/src/forum/components/NotificationsDropdown.js
index 7099ab66f..860189808 100644
--- a/js/src/forum/components/NotificationsDropdown.js
+++ b/js/src/forum/components/NotificationsDropdown.js
@@ -1,5 +1,6 @@
 import Dropdown from '../../common/components/Dropdown';
 import icon from '../../common/helpers/icon';
+import classList from '../../common/utils/classList';
 import NotificationList from './NotificationList';
 
 export default class NotificationsDropdown extends Dropdown {
@@ -9,6 +10,7 @@ export default class NotificationsDropdown extends Dropdown {
     attrs.menuClassName = attrs.menuClassName || 'Dropdown-menu--right';
     attrs.label = attrs.label || app.translator.trans('core.forum.notifications.tooltip');
     attrs.icon = attrs.icon || 'fas fa-bell';
+
     // For best a11y support, both `title` and `aria-label` should be used
     attrs.accessibleToggleLabel = attrs.accessibleToggleLabel || app.translator.trans('core.forum.notifications.toggle_dropdown_accessible_label');
 
@@ -21,7 +23,7 @@ export default class NotificationsDropdown extends Dropdown {
 
     vdom.attrs.title = this.attrs.label;
 
-    vdom.attrs.className += newNotifications ? ' new' : '';
+    vdom.attrs.className = classList(vdom.attrs.className, [newNotifications && 'new']);
     vdom.attrs.onclick = this.onclick.bind(this);
 
     return vdom;
@@ -32,15 +34,15 @@ export default class NotificationsDropdown extends Dropdown {
 
     return [
       icon(this.attrs.icon, { className: 'Button-icon' }),
-      unread ? <span className="NotificationsDropdown-unread">{unread}</span> : '',
+      unread !== 0 && <span className="NotificationsDropdown-unread">{unread}</span>,
       <span className="Button-label">{this.attrs.label}</span>,
     ];
   }
 
   getMenu() {
     return (
-      <div className={'Dropdown-menu ' + this.attrs.menuClassName} onclick={this.menuClick.bind(this)}>
-        {this.showing ? NotificationList.component({ state: this.attrs.state }) : ''}
+      <div className={classList('Dropdown-menu', this.attrs.menuClassName)} onclick={this.menuClick.bind(this)}>
+        {this.showing && NotificationList.component({ state: this.attrs.state })}
       </div>
     );
   }
diff --git a/less/forum/NotificationList.less b/less/forum/NotificationList.less
index 4b4ee2e77..5e7a4d901 100644
--- a/less/forum/NotificationList.less
+++ b/less/forum/NotificationList.less
@@ -1,48 +1,60 @@
-
 .NotificationList {
   overflow: hidden;
-  & .loading-indicator {
-    height: 100px;
-  }
-}
 
-.NotificationList-header {
-  @media @tablet-up {
-    padding: 12px 15px;
-    border-bottom: 1px solid @control-bg;
+  &-header {
+    @media @tablet-up {
+      padding: 12px 15px;
+      border-bottom: 1px solid @control-bg;
 
-    h4 {
-      font-size: 12px;
-      text-transform: uppercase;
-      font-weight: bold;
-      margin: 0;
-      color: @muted-color;
+      display: flex;
+      justify-content: space-between;
+      align-items: center;
+
+      h4 {
+        font-size: 12px;
+        text-transform: uppercase;
+        font-weight: bold;
+        margin: 0;
+        color: @muted-color;
+      }
     }
-  }
-  .Button {
-    float: right;
-    margin-top: -11px;
-    margin-right: -11px;
 
-    // The NotificationList may be displayed inside of the drawer as a
-    // dropdown menu – but the drawer may have .light-contents() applied to
-    // it. In this case we will need to reset the button's styles back to
-    // normal.
-    & when (@config-colored-header = true) {
-      .Button--color(@control-color, @control-bg);
+    // Mark all as read button
+    .Button {
+      padding: 0;
+      text-decoration: none;
 
-      &:hover {
-        color: @link-color;
+      // The NotificationList may be displayed inside of the drawer as a
+      // dropdown menu – but the drawer may have .light-contents() applied to
+      // it. In this case we will need to reset the button's styles back to
+      // normal.
+      & when (@config-colored-header = true) {
+        color: @control-color;
+
+        &:hover,
+        &:focus {
+          color: @link-color;
+        }
+      }
+
+      .add-keyboard-focus-ring();
+      .add-keyboard-focus-ring-offset(4px);
+
+      .icon {
+        margin-right: 0;
       }
     }
   }
+
+  // Message displayed when notifications are empty
+  &-empty {
+    color: @muted-color;
+    text-align: center;
+    padding: 50px 0;
+    font-size: 16px;
+  }
 }
-.NotificationList-empty {
-  color: @muted-color;
-  text-align: center;
-  padding: 50px 0;
-  font-size: 16px;
-}
+
 .NotificationGroup {
   border-top: 1px solid @control-bg;
   margin-top: -1px;
@@ -50,99 +62,143 @@
   &:not(:last-child) {
     margin-bottom: 20px;
   }
-}
-.NotificationGroup-header {
-  font-weight: bold;
-  color: @heading-color !important;
-  padding: 6px 15px;
-  display: block;
-  white-space: nowrap;
-  overflow: hidden;
-  text-overflow: ellipsis;
-}
-.NotificationGroup-badges {
-  margin-left: -2px;
-  margin-right: 18px;
-  vertical-align: 1px;
 
-  .Badge {
-    margin-right: -13px;
-    position: relative;
-    .Badge--size(21px);
+  &-header {
+    font-weight: bold;
+    color: @heading-color !important;
+    padding: 8px 16px;
+    white-space: nowrap;
+    overflow: hidden;
+    text-overflow: ellipsis;
+
+    display: flex;
+    align-items: center;
+
+    // Prevent outline overflowing parent
+    .add-keyboard-focus-ring-offset(-1px);
+  }
+
+  &-badges {
+    @overlap: 13px;
+
+    margin-right: 8px;
+    padding-right: @overlap;
+
+    .Badge {
+      margin-right: -@overlap;
+      position: relative;
+      .Badge--size(21px);
+    }
+  }
+
+  &-content {
+    list-style: none;
+    margin: 0;
+    padding: 0;
   }
 }
-.NotificationGroup-content {
-  list-style: none;
-  margin: 0;
-  padding: 0;
-}
+
 .Notification {
-  display: block;
-  padding: 8px 15px 8px 70px;
+  padding: 8px 16px;
   color: @muted-color !important; // required to override .light-contents applied to header
   overflow: hidden;
 
-  .unread& {
+  display: grid;
+  grid-template-columns: auto auto 1fr auto;
+
+  grid-template-areas:
+    "avatar icon title   button"
+    "x      x    excerpt excerpt";
+
+  align-items: baseline;
+  row-gap: 1px;
+  column-gap: 6px;
+
+  // Prevent outline overflowing parent
+  .add-keyboard-focus-ring-offset(-1px);
+
+  &.unread {
     background: @control-bg;
   }
-  &:hover {
+
+  &:hover,
+  &:focus,
+  &:focus-within {
     text-decoration: none;
     background: @control-bg;
-    
+
     .Notification-action {
-      display: block;
+      opacity: 1;
     }
   }
+
   .Avatar {
     .Avatar--size(24px);
-    float: left;
-    margin: -2px 0 -2px -55px;
+    grid-area: avatar;
   }
+
+  &-icon {
+    font-size: 14px;
+    grid-area: icon;
+  }
+
+  &-title {
+    grid-area: title;
+    display: flex;
+    flex-direction: row;
+    flex-wrap: wrap;
+    align-items: baseline;
+  }
+
+  &-content {
+    line-height: 19px;
+    margin-right: 8px;
+
+    .username {
+      font-weight: bold;
+    }
+  }
+
   time {
+    line-height: inherit;
     font-size: 11px;
+    line-height: 19px;
     font-weight: bold;
     text-transform: uppercase;
   }
 
-  .Notification-action {
-    float: right;
-    display: none;
-    margin-top: -7px;
-    margin-right: -10px;
+  &-action {
     line-height: inherit;
-    padding: 5px 0;
+    padding: 0;
+    opacity: 0;
 
-    & when (@config-colored-header = true) {
-      .Button--color(@control-color, @control-bg);
+    .add-keyboard-focus-ring();
+    .add-keyboard-focus-ring-offset(4px);
 
-      &:hover {
+    grid-area: button;
+
+    // Needs more specificity to fix hover/focus styles not applying in dropdown
+    .Notification & when (@config-colored-header = true) {
+      color: @control-color;
+
+      &:hover,
+      &:focus {
         color: @link-color;
       }
     }
 
     .icon {
-      font-size: 12px;
+      font-size: 13px;
+      margin-right: 0;
     }
   }
-}
-.Notification-icon {
-  float: left;
-  margin-left: -23px;
-  font-size: 14px;
-  margin-top: 2px;
-}
-.Notification-content {
-  margin-right: 5px;
 
-  .username {
-    font-weight: bold;
+  &-excerpt {
+    grid-area: excerpt;
+    color: @muted-more-color;
+    font-size: 11px;
+    white-space: nowrap;
+    text-overflow: ellipsis;
+    overflow: hidden;
   }
 }
-.Notification-excerpt {
-  color: @muted-more-color;
-  font-size: 11px;
-  margin-top: 3px;
-  white-space: nowrap;
-  text-overflow: ellipsis;
-  overflow: hidden;
-}