React header #6197

fhlavac · 2019-09-17T14:49:07Z

Added parameter for element specification to ReactjsHelper
Rewritten help and user menu structure generating (similar as for vertical navbar - should be merged)
Rewritten top-navbar component and divided it into components: user-options, configuration, help, notifications, custom-logo
Created right-section component containing all above mentioned small components
Created top-navbar tests

This PR is dependent on #5854

In notifications part I currently call setUnreadCount (function returning number of unread notifications) by Redux. Previously, it was solved in Angular.
Currently: When number of unread notifications in drawer is updated, Rx message is sent to React header and Redux dispatch updates value stored in Redux store.

epwinchell · 2019-09-17T18:11:48Z

@miq-bot add_reviewer @martinpovolny

epwinchell · 2019-09-17T18:12:00Z

@miq-bot add_reviewer @epwinchell

epwinchell · 2019-09-17T18:12:38Z

@miq-bot add_reviewer @Hyperkid123

martinpovolny · 2019-09-18T08:28:15Z

Touches notifications, pinging @skateman .

Also the notifications should be converted to React as a priority.

This is a piece of Angular that is loaded on every page. Let's avoid that soon. Toolbars will be gone in a moment, then GTL will be replaced and trees. There should be no angular on most pages in the next release.

skateman · 2019-09-18T08:33:48Z

@martinpovolny the pf-react implementation of the drawer is very far from our needs. We did some customizations with @himdel on top of the angular-based implementation and we would have to do that and a little more with the react one before we're able to actually consume it.

I was looking into this last year and it seemed like a painful task. But I'm in the area already because of the popup throttling, so I'll re-evaluate this as well.

Gonna take a look at this piece of code of course ... gimme some 🕐

skateman

Great job, a few problems and some stuff open for debate...

skateman · 2019-09-18T08:34:19Z

app/assets/stylesheets/main.scss

@@ -66,5 +66,4 @@ $login-container-details-border-color-rgba: rgba(0, 0, 0, 0.5);
 body.login.whitelabel {
  background: url($img-bg-login-whitelabel);
  background-size: 100% auto;
-}
-
+}


Missing newline here...

skateman · 2019-09-18T08:38:39Z

app/helpers/application_helper/navbar.rb

+        :items          => item.items.to_a.map(&method(:item_to_hash)),
+        :visible        => item.visible?,
+        :link_params    => item.link_params
+      }


You could implement this a little prettier:

%i[ id name icon placement before type href parent_id feature rbac_feature defaults link_params ].each_with_object(:visible => item.visible?, :items => item.items.to_a.map(&method(:item_to_hash))) do |obj, key| obj[key] = item.try(key) end

Except of visible and items all the fields map nicely to the keys...

10 points to Griffindor for the &method btw 🥇 😆

skateman · 2019-09-18T08:41:59Z

app/javascript/components/top-navbar/right-section.jsx

+    <Help helpMenu={helpMenu} />
+    { currentUser && (
+      <React.Fragment>
+        <Configuration opsExplorerAllowed={opsExplorerAllowed} />


TBH I don't see a reason why to have this as a separate component in a separate file, this is way too small for that and you're creating much bigger ones here. I'd just extract it into a function, but don't take this seriously and ask someone who actually knows React 😉

This was actually by my command! ☠️

But it is better to make these component as small and independent as possible. It is much easier to find them, test them and fix them if need be.

Then IMO the other one(s) should be small(er) as well... It's just feels weird that you have one small, and the rest is HUGE.

skateman · 2019-09-18T08:42:20Z

app/javascript/components/top-navbar/right-section.jsx

+  customLogo, currentUser, helpMenu, opsExplorerAllowed, applianceName, miqGroups, currentGroup, userMenu,
+}) => (
+  <React.Fragment>
+    <CustomLogo customLogo={customLogo} />


Same as below...

skateman · 2019-09-18T08:43:09Z

app/assets/javascripts/controllers/header/header_controller.js

@@ -47,6 +53,7 @@ angular.module('ManageIQ').controller('headerController', ['$scope', 'eventNotif

    vm.notificationsIndicatorTooltip = miqFormatNotification(__('%{count} unread notifications'),
      {count: notificationCount});
+      ManageIQ.redux.store.dispatch({ type: '@@notifications/setUnreadCount', payload: notificationCount.text });


Could you please write a few words into the PR's description about this idea. I know how it works because I made it up 😆 but others might not...

skateman · 2019-09-18T08:46:48Z

app/assets/javascripts/services/event_notifications_service.js

@@ -91,6 +91,11 @@ angular.module('ManageIQ').service('eventNotifications', ['$timeout', '$window',
          });

          updateUnreadCount(events);
+          var x = 0;
+          _.forEach(state.groups, function(group) {


I think you don't need the _ here, you can directly call forEach on the array. Anyway, this code is a great example for using reduce on the array instead of having the helper variable.

skateman · 2019-09-18T08:50:58Z

app/helpers/application_helper/navbar.rb

@@ -1,5 +1,33 @@
 module ApplicationHelper
  module Navbar
+    def menu_to_json(position)


For now, it's okay to have this method here, but after this is done, we should extract it into the Menu::Manager. I have an idea about that, just let's not forget it so we need to talk when this is done and we're both in the office.

skateman · 2019-09-18T08:51:21Z

app/views/layouts/_right_section.html.haml

@@ -0,0 +1 @@
+= react('RightSection', { :customLogo => ::Settings.server.custom_logo, :currentUser => current_user, :helpMenu => menu_to_json(:help),:opsExplorerAllowed => ApplicationHelper.role_allows?(:feature => 'ops_explorer', :any => true), :applianceName => appliance_name, :miqGroups => current_user.miq_groups, :currentGroup => current_group, :userMenu => menu_to_json(:top_right) }, {:class => "nav navbar-nav navbar-right navbar-iconic"}, 'ul')


Missing newline

Maybe it's completely wrong and maybe out of the context of this PR, but could we preseed these data into the Redux store upon login? Then we would have them cached and the list of props would be simpler/shorter/nicer...

skateman · 2019-09-18T08:55:53Z

app/javascript/miq-redux/store.js

+  unreadCount: 0,
+};
+
+const nPrefix = '@@notifications/';


It's just a semantic nitpick and others might disagree with me, but I'd prefer singular here. Just to be a little more future-prooof. Also the setUnreadCount is a little too specific and there's a 100% chance it will be dropped in the future in favor of something like new. So I'd just go with @@notification/new instead, but this is also something where others might and should stop me.

Honestly i don't really care about this :D As long as its same everywhere I am ok with both.

Yeah, in this case let's go with singular please.

epwinchell

Tested. Looks good.

Hyperkid123 · 2019-09-30T07:30:31Z

app/assets/javascripts/services/event_notifications_service.js

@@ -91,6 +91,8 @@ angular.module('ManageIQ').service('eventNotifications', ['$timeout', '$window',
          });

          updateUnreadCount(events);
+          const totalUnreadCount = state.groups.reduce((sum, group) => (sum + group.unreadCount), 0);


I don't think that we can use Array.prototype.reduce in old JS. This is not ES6 and its not being compiled by webpack.

es6-shim. We can use any ES6 libary feature just fine, only the syntax ones have to go through babel.

@fhlavac this is the one ^.. reduce is fine, but arrow functions are new syntax

Oh, and also const! :)

Hyperkid123 · 2019-09-30T07:31:00Z

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

@@ -0,0 +1,19 @@
+import React from 'react';
+
+const Configuration = ({


I don't see any prop types validation here.

Hyperkid123 · 2019-09-30T07:31:42Z

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

+import React from 'react';
+import { Dropdown, Icon, MenuItem } from 'patternfly-react';
+
+const Help = ({


Missing prop types again.

Hyperkid123 · 2019-09-30T07:32:27Z

app/javascript/components/top-navbar/user-options.jsx

+import React from 'react';
+import { Dropdown, Icon, MenuItem } from 'patternfly-react';
+
+const UserOptions = ({


Hyperkid123 · 2019-09-30T07:34:44Z

app/javascript/packs/component-definitions-common.js

@@ -70,6 +76,12 @@ ManageIQ.component.addReact('TagGroup', props => <TagGroup {...props} />);
 ManageIQ.component.addReact('TagView', TagView);
 ManageIQ.component.addReact('TaggingWrapperConnected', TaggingWrapperConnected);
 ManageIQ.component.addReact('TextualSummaryWrapper', TextualSummaryWrapper);
+ManageIQ.component.addReact('RightSection', RightSection);


These should be sorted alphabetically by the component name. It helps us to avoid constant PR conflicts when registering new react components.

martinpovolny · 2019-10-04T12:51:04Z

Looks good!

Please, do not forget to move menu_to_json.
I think it's ok to have small and big components either in one or in multiple files. It really does not bother me for small React components.

Thx, @fhlavac, @Hyperkid123, @skateman !

himdel · 2019-10-22T20:46:15Z

app/javascript/components/top-navbar/user-options.jsx

+                  id={group.description}
+                  key={group.id}
+                  title={__('Change to this Group')}
+                  onClick={(e) => { e.preventDefault(); miqSparkleOn(); miqToggleUserOptions(group.id); }}


This will need a change once #6325 gets merged.

e.preventDefault(); miqChangeGroup(group.id); should do it, just make sure the id is a string please ;)

himdel · 2019-10-22T21:07:01Z

app/javascript/components/top-navbar/user-options.jsx

+      <MenuItem
+        id="logout-btn"
+        href="/dashboard/logout"
+        onClick={event => !miqCheckForChanges() && event.preventDefault()}


And this also needs updating - this time for #6065 (merged in August)

I suggest

(e) => miqCheckForChanges() ? (ManageIQ.logoutInProgress = true) : e.preventDefault()

(Maybe you should go through the git log of all the files removed here and check for recent changes just to make sure ;).)

During the merge of menu_to_json() some props were renamed

Update after different menu_to_json() implementations merge

currentGroup should always be defined

Also move globals for ES-lint to .eslintrc.json

Prefix top-navbar components with 'nav.' in component-definitions-common

Add create(:user_with_group) to cloud_network, cloud_subnet controller

miq-bot · 2019-12-04T08:41:52Z

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

martinpovolny · 2019-12-04T10:41:47Z

Restarted travis to recover from unrelated failures.

himdel · 2019-12-04T14:50:53Z

LGTM, tested vertical menu still working, group switcher, the usual buttons .. everything looks fine 👍

skateman · 2019-12-04T14:54:26Z

🎉

Hyperkid123 · 2019-12-04T15:29:47Z

@fhlavac good job. Now to the next bit 😄

martinpovolny · 2019-12-04T15:42:02Z

❤️ 😍 💟 👍

fhlavac changed the title ~~React header~~ [WIP] React header Sep 17, 2019

miq-bot added the wip label Sep 17, 2019

miq-bot requested review from martinpovolny and epwinchell September 17, 2019 18:12

miq-bot requested a review from Hyperkid123 September 17, 2019 18:13

skateman suggested changes Sep 18, 2019

View reviewed changes

epwinchell approved these changes Sep 18, 2019

View reviewed changes

miq-bot added the unmergeable label Sep 23, 2019

Hyperkid123 reviewed Sep 30, 2019

View reviewed changes

fhlavac force-pushed the react_header branch from ac80223 to f884e8b Compare October 2, 2019 11:21

fhlavac changed the title ~~[WIP] React header~~ React header Oct 2, 2019

miq-bot added unmergeable and removed wip unmergeable labels Oct 2, 2019

himdel mentioned this pull request Oct 3, 2019

Vertical navbar #5854

Merged

fhlavac force-pushed the react_header branch from f884e8b to aec3d54 Compare October 3, 2019 14:21

miq-bot removed the unmergeable label Oct 3, 2019

himdel reviewed Oct 22, 2019

View reviewed changes

miq-bot added the unmergeable label Oct 23, 2019

fhlavac added 17 commits December 4, 2019 09:35

Update recursive-props after props renaming

4985e73

During the merge of menu_to_json() some props were renamed

Main-menu and top-navbar tests update

2e48cd6

Update after different menu_to_json() implementations merge

Remove currentGroup defined condition in user-opt

03e5893

currentGroup should always be defined

Change const to var in event_notification_service

ae1258f

Add target to HelpMenu item

8cb4c5b

Remove => function from event_notification_service

bb0dedb

Update snapshots

382979d

Move getTargetByType function to global helpers

0623dc2

Use getTargetByType function in help-menu

5716ce6

Make currentUser required in props validation

de2e175

Also move globals for ES-lint to .eslintrc.json

Convert ids of menu items to string

f25dd2c

Move notificationReducer to a separate file

e50bdb0

Prefix top-navbar components defs with 'navbar.'

9ba4e24

Prefix top-navbar components with 'nav.' in component-definitions-common

Add prefix "navbar." to component names string

aff7b17

Add create(:user_with_group) to controllers

e98b1d5

Add create(:user_with_group) to cloud_network, cloud_subnet controller

Add :user_with_group floating_ip_controller

d31458e

Add prefix "navbar." in _right_section.html.haml

0ca1e44

fhlavac force-pushed the react_header branch from bf6b7a8 to 0ca1e44 Compare December 4, 2019 08:37

himdel merged commit 49c2416 into ManageIQ:master Dec 4, 2019

himdel added this to the Sprint 126 Ending Dec 9, 2019 milestone Dec 4, 2019

martinpovolny added changelog/yes react labels Dec 4, 2019

himdel mentioned this pull request Feb 26, 2020

Fix update of vertical menu styles after menu click ManageIQ/manageiq-v2v#1110

Merged

himdel mentioned this pull request May 1, 2020

ManageIQ Navigation design #6986

Closed

		@@ -0,0 +1 @@
		= react('RightSection', { :customLogo => ::Settings.server.custom_logo, :currentUser => current_user, :helpMenu => menu_to_json(:help),:opsExplorerAllowed => ApplicationHelper.role_allows?(:feature => 'ops_explorer', :any => true), :applianceName => appliance_name, :miqGroups => current_user.miq_groups, :currentGroup => current_group, :userMenu => menu_to_json(:top_right) }, {:class => "nav navbar-nav navbar-right navbar-iconic"}, 'ul')

		@@ -0,0 +1,19 @@
		import React from 'react';

		const Configuration = ({

React header #6197

React header #6197

Conversation

fhlavac commented Sep 17, 2019 • edited Loading

epwinchell commented Sep 17, 2019

epwinchell commented Sep 17, 2019

epwinchell commented Sep 17, 2019

martinpovolny commented Sep 18, 2019

skateman commented Sep 18, 2019

skateman 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epwinchell 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinpovolny commented Oct 4, 2019

Choose a reason for hiding this comment

himdel Oct 22, 2019 • edited Loading

Choose a reason for hiding this comment

miq-bot commented Dec 4, 2019

martinpovolny commented Dec 4, 2019

himdel commented Dec 4, 2019

skateman commented Dec 4, 2019

Hyperkid123 commented Dec 4, 2019 • edited Loading

martinpovolny commented Dec 4, 2019

fhlavac commented Sep 17, 2019 •

edited

Loading

himdel Oct 22, 2019 •

edited

Loading

Hyperkid123 commented Dec 4, 2019 •

edited

Loading