Move $().setupVerticalNavigation(...) logic to React #6463

fhlavac · 2019-11-28T14:37:56Z

deleted $().setupVerticalNavigation(...) method from miq_application.js
moved vertical-navbar logic for levels expanding to React components
replaced left-section (collapsed state toggle + miq logo) with NavbarHeader React component
moved vertical-navbar collapsing logic to React
updated tests for vertical-nav collapsing using Redux
removed pinning from vertical-navbar and related styling

@epwinchell Do you see anything about removing the pinning what should be changed or fixed?

Hyperkid123 · 2019-11-29T07:34:54Z

@miq-bot assign @martinpovolny
@miq-bot add_label react
@miq-bot add_reviewer @Hyperkid123

Hyperkid123

I think the code works. Have some minor remarks for the styling. As always I am probably not aware of all the features the has. Somebody that is should probably double check if anything is missing. @skateman @himdel

Hyperkid123 · 2019-11-29T07:39:26Z

app/javascript/components/main-menu/second-level.jsx

+        'menu-list-group-item',
+        {
+          'tertiary-nav-item-pf': hasSubitems,
+          'is-hover': useContext(HoverContext).secondLevelId === id,


This is usage of hooks is dangerous. 1st rule of rule of react hooks is that they have to defined at the top of the function to avoid potential state inconsistencies.

const Foo = () => { const { bar } = useSomething() // same as const bar = useSomething().bar but it is preferred syntax. return ( <div> ... ) }

Granted this is not the case for useContext but still, we should follow the rules not to confuse anyone who might use this as a inspiration in the future.

@fhlavac : can you move the useContext into a separate statement after/before line 20?

@Hyperkid123 : that is what you are asking for, right?

Hyperkid123 · 2019-11-29T07:40:18Z

app/javascript/components/main-menu/third-level.jsx

@@ -10,7 +11,10 @@ const ThirdLevel = ({
  visible,
  type,
 }) => (!visible ? null : (
-  <li className={`menu-list-group-item ${active ? 'active' : ''}`} id={`menu_item_${id}`}>
+  <li
+    className={`menu-list-group-item ${active ? 'active' : ''}`}


imported clsx but its not used for composite className

Hyperkid123 · 2019-11-29T07:40:35Z

app/javascript/components/main-menu/top-level.jsx

+          'menu-list-group-item',
+          'secondary-nav-item-pf',
+          {
+            'is-hover': useContext(HoverContext).topLevelId === id,


again hooks rules

Hyperkid123 · 2019-11-29T07:42:58Z

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

+          dispatch(toggleVerticalMenuCollapsed());
+          const content = window.document.getElementsByClassName('container-pf-nav-pf-vertical-with-sub-menus')[0];
+          if (isVerticalMenuCollapsed) {
+            content.classList.remove('collapsed-nav');


I think it would be nice to check if the content exists. I don't know if its possible for the content no to be present in the DOM but its a good practice to first check if the element really exists before trying to call any functions on it.

Hyperkid123 · 2019-11-29T07:46:09Z

app/javascript/components/main-menu/main-menu.jsx

+
+const MainMenu = ({ menu }) => {
+  const [activeIds, setActiveIds] = useState({});
+  const isVerticalMenuCollapsed = useSelector(store => store.menuReducer.isVerticalMenuCollapsed);


I would refer if you use object destructing for the property:

const isVerticalMenuCollapsed = useSelector(({ menuReducer: { isVerticalMenuCollapsed }}) => isVerticalMenuCollapsed);

skateman · 2019-11-29T10:15:03Z

package.json

@@ -43,6 +43,7 @@
    "bootstrap-filestyle": "~1.2.1",
    "bootstrap-switch": "3.3.4",
    "classnames": "~2.2.6",
+    "clsx": "^1.0.4",


Just one line above, we pull in the classnames package which does the same thing as clsx.

in ui-classic, we only have one use of classnames, which should be trivial to change.

app/javascript/forms/miq-button.jsx 3:import ClassNames from 'classnames'; 14: const klass = ClassNames({

But, in v2v, there's 28 more uses of classnames.

So, I'd suggest just going with classnames, unless you want to update https://github.com/ManageIQ/manageiq-v2v too.

@fhlavac ok classnames it is. Can you please change it? The package has almost the same syntax as clsx. You can find it in the docs.

Hyperkid123 · 2019-12-01T12:33:08Z

@skateman oh did not noticed that. I don't even know if classnames were ever used. @fhlavac can you check if we use classnames? If we already use classnames just use that instead and remove clsx. If not i would just remove classnames. I do prefer clsx because its a bit more lightweight.

skateman · 2019-12-01T13:18:59Z

My point is to use one, doesn't matter which 😆

epwinchell · 2019-12-03T15:24:40Z

@epwinchell Do you see anything about removing the pinning what should be changed or fixed?

@fhlavac Nope, it looks fine.

epwinchell

Tested. Looks fine. I do see a couple of issues with the menu in the mobile layout that are unrelated to this PR. We can address seperately.

martinpovolny · 2019-12-04T10:53:08Z

@fhlavac : there's just a few minor comments. We are almost there.

Thanks for the great work everyone!

Disable setupVerticalNavigation() and convert expanding logic to React.

himdel · 2019-12-04T15:07:59Z

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

@@ -65,6 +66,7 @@ ManageIQ.component.addReact('ImportDatastoreViaGit', ImportDatastoreViaGit);
 ManageIQ.component.addReact('MainMenu', MainMenu);
 ManageIQ.component.addReact('MiqAboutModal', MiqAboutModal);
 ManageIQ.component.addReact('MiqToolbar', MiqToolbar);
+ManageIQ.component.addReact('NavbarHeader', NavbarHeader);


@fhlavac Can you use the navbar. convention here too? :)

himdel · 2019-12-04T15:08:15Z

app/views/layouts/_navbar-header.html.haml

@@ -0,0 +1 @@
+= react('NavbarHeader', { :customBrand => ::Settings.server.custom_brand, :imagePath => image_path("layout/brand.svg") })


in component-definitions-common.js

himdel · 2019-12-04T16:06:20Z

The menu currently doesn't remember the collapsed state...

click the kebab to make the menu small
click the menu to go to some other screen
the menu will load full-sized

It should remember and load in the collapsed state.

The original implementation is using localStorage['patternfly-navigation-primary'] = "collapsed" | "expanded", maybe you can reuse the same variable?

Hyperkid123 · 2019-12-04T16:12:43Z

The menu currently doesn't remember the collapsed state...

click the kebab to make the menu small

click the menu to go to some other screen

the menu will load full-sized

It should remember and load in the collapsed state.

The original implementation is using localStorage['patternfly-navigation-primary'] = "collapsed" | "expanded", maybe you can reuse the same variable?

Yeah redux state is not persisted on full page refresh. So we will have to use local storage. We might want to think about persisting redux state to local storage further(somewhere else). But i don't know how i feel about that.

himdel · 2019-12-04T16:14:18Z

But i don't know how i feel about that.

I don't think it makes sense to persist the whole redux state, that sounds like it would always be caching more than it should.

IMO we should have explicit interfaces for specific things that should remain set after reload, this may be one of them.

Hyperkid123 · 2019-12-04T16:14:52Z

IMO we should have explicit interfaces for specific things that should remain set after reload, this may be one of them.

agreed

miq-bot · 2019-12-05T14:46:47Z

Checked commits fhlavac/manageiq-ui-classic@d794a15~...11c062d 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 · 2019-12-05T14:51:54Z

app/javascript/components/main-menu/helpers.js

+  const content = window.document.getElementsByClassName('container-pf-nav-pf-vertical-with-sub-menus')[0];
+  if (content) {
+    if (isVerticalMenuCollapsed) {
+      content.classList.add('collapsed-nav');


It's a bit unfortunate we have to manipulate these classes manually, since this is a react component and the data is in state. Any way to use react here? :)

Oh, never mind, that particular element is in app/views/layouts/_content.html.haml and app/views/layouts/_center_div_*.html.haml, and is still outside of the react navbar.

So, this makes sense for now, no issues :)

yeah you would have to have the whole layout as a react component. No way around it for now i am afraid

Hyperkid123 reviewed Nov 29, 2019

View reviewed changes

skateman reviewed Nov 29, 2019

View reviewed changes

martinpovolny assigned skateman Dec 2, 2019

martinpovolny added refactoring technical debt labels Dec 2, 2019

himdel mentioned this pull request Dec 2, 2019

[WIP] Don't let angular-patternfly rewrite ui-components lodash #6449

Closed

himdel requested a review from epwinchell December 2, 2019 14:57

miq-bot assigned martinpovolny and unassigned skateman Dec 2, 2019

miq-bot added the react label Dec 2, 2019

miq-bot requested a review from Hyperkid123 December 2, 2019 15:03

epwinchell approved these changes Dec 3, 2019

View reviewed changes

fhlavac changed the title ~~Move $().setupVerticalNavigation(...) logic to React~~ [WIP] Move $().setupVerticalNavigation(...) logic to React Dec 4, 2019

fhlavac force-pushed the disable_setup branch from 8db0b3d to b7c24ba Compare December 4, 2019 12:20

miq-bot added the wip label Dec 4, 2019

fhlavac changed the title ~~[WIP] Move $().setupVerticalNavigation(...) logic to React~~ Move $().setupVerticalNavigation(...) logic to React Dec 4, 2019

miq-bot removed the wip label Dec 4, 2019

fhlavac force-pushed the disable_setup branch from f72ca0b to 5144ba2 Compare December 4, 2019 14:21

miq-bot added the unmergeable label Dec 4, 2019

fhlavac added 6 commits December 4, 2019 15:56

Convert vertical-navbar levels expanding to React

d794a15

Disable setupVerticalNavigation() and convert expanding logic to React.

Update snapshot after converting expanding logic

e8d5430

Replace navbar-header with empty partial

78e3228

Add NavbarHeader component design implementation

0511504

Move navbar-header component to top-navbar dir

d74d567

Create menu-reducer for navbar collapsed state

fe5dc1e

fhlavac added 12 commits December 4, 2019 16:07

Fix content resizing when vertical-nav collapsed

1d89099

Delete vertical navbar item pinning

003badf

Clean classNames using clsx

852b0aa

Add tests for vertical-nav collapsing using Redux

d2a8832

Shorten lines in vertical-nav tests

e84d60c

Delete setupVerticalNavigation()

ff6f654

Remove styling for menu-item pinning

9650d2f

Move hooks definitions to the top of the function

bd734f0

Update className for top-level-item

8dc77df

Replace 'clsx' package usage with 'classnames'

31b9bc4

Verify content element existence before using it

eefb30c

Add object destructuring to navbar-header

7e28be0

himdel reviewed Dec 4, 2019

View reviewed changes

fhlavac added 2 commits December 4, 2019 16:17

Rename 'reducer' to 'menuReducer'

830e454

Add 'navbar.' prefix to NavbarHeader

4afbcfb

in component-definitions-common.js

fhlavac force-pushed the disable_setup branch from 5144ba2 to 4afbcfb Compare December 4, 2019 15:52

himdel removed the unmergeable label Dec 4, 2019

Preserve collapsed state using Local Storage

11c062d

himdel reviewed Dec 5, 2019

View reviewed changes

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

himdel merged commit 082a476 into ManageIQ:master Dec 5, 2019

himdel mentioned this pull request Feb 26, 2020

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move $().setupVerticalNavigation(...) logic to React #6463

Move $().setupVerticalNavigation(...) logic to React #6463

fhlavac commented Nov 28, 2019

Hyperkid123 commented Nov 29, 2019

Hyperkid123 left a comment

Hyperkid123 Nov 29, 2019

martinpovolny Dec 4, 2019

fhlavac Dec 4, 2019

Hyperkid123 Nov 29, 2019

Hyperkid123 Nov 29, 2019

Hyperkid123 Nov 29, 2019

Hyperkid123 Nov 29, 2019

skateman Nov 29, 2019

himdel Dec 2, 2019 •

edited

Hyperkid123 Dec 3, 2019

Hyperkid123 commented Dec 1, 2019

skateman commented Dec 1, 2019

epwinchell commented Dec 3, 2019

epwinchell left a comment

martinpovolny commented Dec 4, 2019

himdel Dec 4, 2019

himdel Dec 4, 2019

himdel commented Dec 4, 2019 •

edited

Hyperkid123 commented Dec 4, 2019

himdel commented Dec 4, 2019

Hyperkid123 commented Dec 4, 2019

miq-bot commented Dec 5, 2019

himdel Dec 5, 2019

himdel Dec 5, 2019

Hyperkid123 Dec 10, 2019

		@@ -0,0 +1 @@
		= react('NavbarHeader', { :customBrand => ::Settings.server.custom_brand, :imagePath => image_path("layout/brand.svg") })

Move $().setupVerticalNavigation(...) logic to React #6463

Move $().setupVerticalNavigation(...) logic to React #6463

Conversation

fhlavac commented Nov 28, 2019

Hyperkid123 commented Nov 29, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel Dec 2, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hyperkid123 commented Dec 1, 2019

skateman commented Dec 1, 2019

epwinchell commented Dec 3, 2019

epwinchell left a comment

Choose a reason for hiding this comment

martinpovolny commented Dec 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel commented Dec 4, 2019 • edited

Hyperkid123 commented Dec 4, 2019

himdel commented Dec 4, 2019

Hyperkid123 commented Dec 4, 2019

miq-bot commented Dec 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel Dec 2, 2019 •

edited

himdel commented Dec 4, 2019 •

edited