Notification drawer #6644

fhlavac · 2020-01-30T13:09:54Z

Notification drawer → React

rewritten notification drawer into React
rewritten toast notifications into React
created tests for notification-reducer, notification-drawer, notifications-actions, toast-list
updated top navbar tests to reflect state management changes in the notification drawer
deleted the old Angular implementation

A number of all user's notifications is now fetched using a separate API request since data GET request returns a wrong COUNT - it should be fixed after ManageIQ/manageiq-api#726

I'm not sure about displaying all the necessary attributes in toast notifications (especially the link).
@skateman, could you please check it?

fhlavac · 2020-01-30T13:12:23Z

@miq-bot add_reviewer @skateman

miq-bot · 2020-01-30T13:13:07Z

@fhlavac Cannot add the following reviewer because they are not recognized: skateman Hyperkid123 martinpovolny

fhlavac · 2020-01-30T13:14:21Z

@miq-bot add_reviewer @Hyperkid123

fhlavac · 2020-01-30T13:15:31Z

@miq-bot add_reviewer @martinpovolny

Hyperkid123 · 2020-01-30T14:09:59Z

@fhlavac to fix the Travis issues. You will have to delete tests for the old angular controllers. Also, there is a problem with dates in your snapshots. I think an update should be good enough. If it won't help just remove the snapshot.

skateman · 2020-01-30T14:27:50Z

I like it even without looking at the code, my only concern is if the kebab context menu things are working as they used to work in the old implemenation.

Hyperkid123 · 2020-01-30T14:40:30Z

@skateman do we know how they suppose to work? Can the session thing be deleted?

Hyperkid123

So this is what I have found so far.

Hyperkid123 · 2020-01-30T14:47:52Z

app/javascript/components/notification-drawer/notification-drawer.jsx

+  const [isPanelExpanded, setPanelExpanded] = useState(true);
+  const unreadCount = useSelector(({ notificationReducer: { unreadCount } }) => unreadCount);
+  const notifications = useSelector(({ notificationReducer: { notifications } }) => notifications);
+  const totalNotificationsCount = useSelector(({ notificationReducer: { totalNotificationsCount } }) => totalNotificationsCount);


useSelector can be used for multiple values. You don't need to call the hook each time you need something from the state.

const { isDrawerVisible, unreadCount } = useSelector(({ notificationReducer: { isDrawerVisible, unreadCount } }) => ({ isDrawerVisible, unreadCount )});

All state variables should probably use only one useSelector if possible.

Hyperkid123 · 2020-01-30T14:50:32Z

app/javascript/components/toast-list/toast-list.jsx

+const ToastList = () => {
+  const dispatch = useDispatch();
+  const toastNotifications = useSelector(({ notificationReducer: { toastNotifications } }) => toastNotifications);
+  const notificationTimerDelay = 8000;


I would define this constant outside of the component. If you leave it is at is, it will create a new variable for each notification which uses slightly more memory. It's not a big deal but its a good practice.

Hyperkid123 · 2020-01-30T14:51:09Z

app/javascript/components/top-navbar/notifications.jsx

+        className="nav-item-iconic drawer-pf-trigger-icon"
+        title={`${unreadCount} ${__('unread notifications')}`}
+        onClick={() => {
+          dispatch({ type: '@@notifications/toggleDrawerVisibility' });


You should move this string to a constant somewhere.

Hyperkid123 · 2020-01-30T14:53:53Z

app/javascript/spec/fixtures/notifications.json

+    "type": "success",
+    "unread": true
+  }
+]


Missing newline at the end of the file

Hyperkid123 · 2020-01-30T14:54:01Z

app/javascript/spec/fixtures/resources.json

+    "seen": false,
+    "href": "http://localhost:3000/api/notifications/10000000017255"
+  }]
+}


Hyperkid123 · 2020-01-30T14:54:42Z

package.json

@@ -63,7 +63,7 @@
    "moment-strftime": "~0.5.0",
    "moment-timezone": "~0.5.21",
    "numeral": "~2.0.6",
-    "patternfly": "~3.59.1",
+    "patternfly": "~3.59.4",


Can you revert it back to the original version? I think this upgrade did not help anything.

skateman · 2020-01-31T09:25:07Z

@skateman do we know how they suppose to work? Can the session thing be deleted?

What do you mean?

Hyperkid123 · 2020-02-03T08:00:15Z

app/javascript/components/notification-drawer/notification-drawer.jsx

+  const {
+    isDrawerVisible, unreadCount, notifications, totalNotificationsCount, maxNotifications,
+  } = useSelector(({
+    notificationReducer: {


There is a lot of extra code duplications. If you have a use case like this, when you are basically consuming the whole state, you can just return the whole object and destruct it only in one place:

const { isDrawerVisible, unreadCount, notifications, totalNotificationsCount, maxNotifications, } = useSelector(({ notificationReducer }) => notificationReducer)

Hyperkid123 · 2020-02-03T08:04:11Z

app/javascript/components/notification-drawer/notification-drawer.jsx

+                                dispatch(toggleMaxNotifications())
+                              }
+                            >
+                              { maxNotifications ? __('Show all (may take a while)') : __(`Show only the first ${maxNotificationsConstant}`)}


This is not going to work. Get text translation (the __() function) does not work with es6 template strings. You should use sprintf if you want to use variables in translations.

__(`Show only the first ${maxNotificationsConstant}`) => sprintf(__(Show only the first %s'), maxNotificationsConstant)

Hyperkid123 · 2020-02-03T08:05:05Z

app/javascript/components/top-navbar/notifications.jsx

+const Notifications = () => {
+  const dispatch = useDispatch();
+  const isDrawerVisible = useSelector(({ notificationReducer: { isDrawerVisible } }) => isDrawerVisible);
+  const unreadCount = useSelector(({ notificationReducer: { unreadCount } }) => unreadCount);


^^ use only one useSelector

Hyperkid123 · 2020-02-03T08:06:19Z

app/javascript/components/top-navbar/notifications.jsx

 };

-export default connect(mapStateToProps)(Notifications);
+export default (Notifications);


No need for extra brackets

Hyperkid123 · 2020-02-03T08:08:16Z

app/javascript/spec/miq-redux/notification-reducer.spec.js

+    maxNotifications,
+  };
+
+  beforeEach(() => {


empty setup and clean callbacks

skateman · 2020-02-04T10:42:10Z

Toasts aren't supposed to be shown when the drawer is open, because they can cover the drawer itself where the redundant information:

fhlavac · 2020-02-04T11:17:14Z

Toasts aren't supposed to be shown when the drawer is open, because they can cover the drawer itself where the redundant information:

@skateman Displaying both doesn't make sense but in the angular implementation drawer and toast notifications were displayed at the same time as well... maybe it is a bug...

Hyperkid123 · 2020-02-04T11:22:26Z

app/javascript/components/notification-drawer/notification-drawer.jsx

+          <Drawer.Title
+            title={drawerTitle}
+            onCloseClick={() => dispatch(toggleDrawerVisibility())}
+            onExpandClick={() => { setDrawerExpanded(!isDrawerExpanded); }}


no need for extra block around one-line function.

onExpandClick={() => setDrawerExpanded(!isDrawerExpanded)}

Hyperkid123 · 2020-02-04T11:22:37Z

app/javascript/components/notification-drawer/notification-drawer.jsx

+                    <a
+                      id="eventsExpander"
+                      className={classnames({ collapsed: !isPanelExpanded })}
+                      onClick={() => { setPanelExpanded(!isPanelExpanded); }}


Hyperkid123

@skateman is the functionality OK?

skateman · 2020-02-04T20:29:00Z

Yeah, except of the toasts, but we can address that in a future PR. There are multiple possible refactoring points in the area.

skateman

fhlavac · 2020-02-06T13:46:13Z

@miq-bot add_reviewer @himdel

miq-bot · 2020-02-10T11:16:05Z

Checked commits fhlavac/manageiq-ui-classic@4f6059b~...88664a3 with ruby 2.5.5, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. 🍰

himdel · 2020-02-18T17:40:19Z

app/javascript/packs/application-common.js

@@ -64,3 +65,5 @@ miqOptimizationInit();

 import * as move from '../helpers/move.js';
 ManageIQ.move = move;
+
+ManageIQ.redux.store.dispatch(initNotifications(true));


This is causing an API request on the login screen, causing a browser login popup.

EDIT: fixing in #6689

miq-bot requested review from skateman and Hyperkid123 January 30, 2020 13:15

miq-bot requested a review from martinpovolny January 30, 2020 13:16

Hyperkid123 reviewed Jan 30, 2020

View reviewed changes

Hyperkid123 reviewed Feb 3, 2020

View reviewed changes

fhlavac force-pushed the notification-drawer branch from b2e5c95 to f0101d0 Compare February 4, 2020 09:02

fhlavac changed the title ~~Notification drawer~~ [WIP]Notification drawer Feb 4, 2020

miq-bot added the wip label Feb 4, 2020

fhlavac changed the title ~~[WIP]Notification drawer~~ Notification drawer Feb 4, 2020

fhlavac closed this Feb 4, 2020

fhlavac reopened this Feb 4, 2020

miq-bot removed the wip label Feb 4, 2020

fhlavac force-pushed the notification-drawer branch from f0101d0 to 64426f5 Compare February 4, 2020 11:09

Hyperkid123 reviewed Feb 4, 2020

View reviewed changes

fhlavac force-pushed the notification-drawer branch from 64426f5 to 8f5f961 Compare February 4, 2020 12:28

Hyperkid123 approved these changes Feb 4, 2020

View reviewed changes

skateman approved these changes Feb 4, 2020

View reviewed changes

miq-bot requested a review from himdel February 6, 2020 13:47

fhlavac added 17 commits February 10, 2020 12:14

Update top-navbar tests to accept new drawer

a64fc7f

Sync local notifications state with API

2f344b5

Create notification drawer tests

a0cf1eb

Create notification reducer tests

8e5f9c8

Sync toast make notification read action with API

98e3108

Fix sync notification read on icon click with API

dbc731e

Fix tests after sync makeRead on icon click

60dd8d3

Add toast-list tests

0ad1462

Add action creators for notifications

4b7f878

Update tests after adding action creators

cdb2adb

Add notifications-actions tests

a016ac6

Fix snapshot time failure

f592216

Delete event_notification_service tests

ab4b93b

Use useSelector hook for multiple values

1b0c599

Change patternfly version back to 3.59.1

f3fdd8c

Clean the code after rewriting the notification drawer

ec46c18

Add "View details" link to the toast notification

88664a3

fhlavac force-pushed the notification-drawer branch from 8f5f961 to 88664a3 Compare February 10, 2020 11:15

himdel added notifications react labels Feb 17, 2020

himdel self-assigned this Feb 17, 2020

h-kataria assigned h-kataria and unassigned himdel Feb 17, 2020

h-kataria added this to the Sprint 130 Ending Feb 17, 2020 milestone Feb 17, 2020

h-kataria merged commit d56553d into ManageIQ:master Feb 17, 2020

h-kataria added ivanchuk/no technical debt labels Feb 17, 2020

himdel reviewed Feb 18, 2020

View reviewed changes

himdel mentioned this pull request Feb 18, 2020

Notification drawer - fix login screen popup #6689

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Notification drawer #6644

Notification drawer #6644

fhlavac commented Jan 30, 2020 •

edited

Loading

fhlavac commented Jan 30, 2020 •

edited

Loading

miq-bot commented Jan 30, 2020

fhlavac commented Jan 30, 2020

fhlavac commented Jan 30, 2020

Hyperkid123 commented Jan 30, 2020

skateman commented Jan 30, 2020

Hyperkid123 commented Jan 30, 2020

Hyperkid123 left a comment

Hyperkid123 Jan 30, 2020

Hyperkid123 Jan 30, 2020

Hyperkid123 Jan 30, 2020

Hyperkid123 Jan 30, 2020

Hyperkid123 Jan 30, 2020

Hyperkid123 Jan 30, 2020

skateman commented Jan 31, 2020

Hyperkid123 Feb 3, 2020

Hyperkid123 Feb 3, 2020

Hyperkid123 Feb 3, 2020

Hyperkid123 Feb 3, 2020

Hyperkid123 Feb 3, 2020

skateman commented Feb 4, 2020

fhlavac commented Feb 4, 2020 •

edited

Loading

Hyperkid123 Feb 4, 2020

Hyperkid123 Feb 4, 2020

Hyperkid123 left a comment

skateman commented Feb 4, 2020

skateman left a comment

fhlavac commented Feb 6, 2020

miq-bot commented Feb 10, 2020

himdel Feb 18, 2020 •

edited

Loading

Notification drawer #6644

Notification drawer #6644

Conversation

fhlavac commented Jan 30, 2020 • edited Loading

fhlavac commented Jan 30, 2020 • edited Loading

miq-bot commented Jan 30, 2020

fhlavac commented Jan 30, 2020

fhlavac commented Jan 30, 2020

Hyperkid123 commented Jan 30, 2020

skateman commented Jan 30, 2020

Hyperkid123 commented Jan 30, 2020

Hyperkid123 left a 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

skateman commented Jan 31, 2020

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

skateman commented Feb 4, 2020

fhlavac commented Feb 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hyperkid123 left a comment

Choose a reason for hiding this comment

skateman commented Feb 4, 2020

skateman left a comment

Choose a reason for hiding this comment

fhlavac commented Feb 6, 2020

miq-bot commented Feb 10, 2020

himdel Feb 18, 2020 • edited Loading

Choose a reason for hiding this comment

fhlavac commented Jan 30, 2020 •

edited

Loading

fhlavac commented Jan 30, 2020 •

edited

Loading

fhlavac commented Feb 4, 2020 •

edited

Loading

himdel Feb 18, 2020 •

edited

Loading