Vertical navbar #5854

fhlavac · 2019-07-23T11:41:02Z

Task

Rewritten structure generation - made it recursive and moved it to navbar.rb
Rewritten and divided React main_menu component into level components
Created recursive PropTypes
Created main-menu tests

fixes https://bugzilla.redhat.com/show_bug.cgi?id=1734661

Hyperkid123 · 2019-07-23T13:17:04Z

Adding a bit of context.

Right now it still uses the same logic as current navigation. This PR changes migrates the haml layout to React and adds a new function that generates JSON fro which the menu is rendered.

In future PR we will move all the show/hide, click/hover and any other logic to the react component. Currently that is handled via pf JS which is triggering these state changes based on css classes (not id or any other unique identifiers)

I know that @epwinchell has some requests to change the current behavior. We would like to introduce them once we start moving the logic to React component. @epwinchell if you could create GH issue with the changes you would like to see that would be great.

Hyperkid123 · 2019-07-23T13:19:19Z

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

+  0: props => <TopLevel level={level} {...props} />,
+  1: props => <SecondLevel level={level} {...props} />,
+  2: props => <ThirdLevel level={level} {...props} />,
+})[level];


I think we should add a fallback component if somebody passes level value other than 0 - 3

const getLevelComponent = level => ({ 0: props => <TopLevel level={level} {...props} />, 1: props => <SecondLevel level={level} {...props} />, 2: props => <ThirdLevel level={level} {...props} />, })[level] || props => <ThirdLevel level={level} {...props} />;

This will always return ThirdLevel component if you gave the function any level that it does not cover.

Hyperkid123 · 2019-07-23T13:21:51Z

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

+  items,
+  level,
+}) => (
+  <li className={`list-group-item ${items.length > 0 && 'tertiary-nav-item-pf'}`} data-target={`#menu-${id}`}>


This will add false to className if the condition is not met. We don't want that.

Hyperkid123 · 2019-07-23T13:24:21Z

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

+        </div>
+        <ul className="list-group">
+          {
+                items.map(props => <MenuItem key={props.id} level={level + 1} {...props} />)


Put this on single line please.

martinpovolny · 2019-07-23T13:24:50Z

Marking this as "WIP" as we do not want it merged (even if it was finished) until the release is out.

Hyperkid123 · 2019-07-23T13:25:45Z

app/views/layouts/_header.html.haml

@@ -15,7 +15,7 @@
      %li.dropdown{"open" => "!hideDrawer"}
        %a{:class => "nav-item-iconic drawer-pf-trigger-icon", :title => "{{vm.notificationsIndicatorTooltip}}", "ng-click" => "vm.toggleNotificationsList()"}
          %span.fa.fa-bell
-          %span{"ng-class" => "{'badge badge-pf-bordered': vm.newNotifications}"} 
+          %span{"ng-class" => "{'badge badge-pf-bordered': vm.newNotifications}"}


I don't think this file should be changed 😉

Hyperkid123 · 2019-07-23T13:28:36Z

@miq-bot add_label react
@miq-bot add_label hammer/no

epwinchell · 2019-07-29T17:59:40Z

@miq-bot add_reviewer @epwinchell

epwinchell

Works great

epwinchell · 2019-07-29T19:17:12Z

@fhlavac Please rebase

epwinchell · 2019-07-29T22:04:15Z

if you could create GH issue with the changes you would like to see that would be great.

https://bugzilla.redhat.com/show_bug.cgi?id=1734181

Hyperkid123

Looks good. @martinpovolny i think you can merge this once the release is out. We will continue with next PR that will extract the logic from PF3. We will have to re do the top Nav too because its controlled by the same PF JS code.

martinpovolny · 2019-07-31T11:08:27Z

Ok, @fhlavac : please un"WIP" the PR.

@epwinchell : good to go?

himdel · 2019-07-31T11:22:59Z

Is this new menu expected to support all the existing features?
(Except for clicking on sections.)

Specifically, I'm not seeing any code dealing with:

calling miqCheckForChanges before leaving a form
opening links in a new window
opening modals (via sendDataWithRx)

himdel · 2019-07-31T11:23:36Z

(This should really not be ignoring anything from link_params, but it only reads the href it seems.)

=> not ready yet IMO

martinpovolny · 2019-07-31T11:52:28Z

@himdel : I sort of expected something like that ;-)

Can we somehow capture the functionality that you pointed out in some tests? Sounds to me like 3 pretty important concepts and all the tests are green.

himdel · 2019-07-31T11:55:05Z

Well, link params provided the whole %a attributes hash in the view. So, it's probably the only place to look at, but also painful to test.

Though, since we're moving to react, a new snapshot test could cover this.

epwinchell · 2019-07-31T13:39:24Z

@martinpovolny yes, reviewed/approved

fhlavac · 2019-08-27T09:32:47Z

@martinpovolny @himdel

Added support for missing types from link_params - "modal", "big_iframe" and "new_window".
(It would be nice to have data where these types occur to be able to really test it. I verified the functionality only by changing some items with "default" type to the other types mentioned.)
Fixed calling miqCheckForChanges before leaving a form.

This adds non expanging markup for top-level items with no child items

skateman · 2019-11-06T14:07:51Z

The CSS can be simplified, the JSON conversion can be refactored, but in a future PR, I'm good with this. Clicked through the UI a little and it all worked, even the custom menu.

skateman

himdel · 2019-11-06T14:33:30Z

Confirming custom menus are working :)

But now everything has an id, not only menu items.
@fhlavac can you please make sure only menu items get ids? Or change the section ids to use the menu_section_ prefix instead of menu_item_?

miq-bot · 2019-11-06T15:27:03Z

Checked commits fhlavac/manageiq-ui-classic@24e56b0~...4eb3f7d with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. ⭐

fhlavac · 2019-11-06T15:30:40Z

Update:

items have menu_item_ prefixes
sections have menu_section_ prefixes
removed redundant data-target at top and second level

himdel · 2019-11-06T15:42:25Z

Merging when green :)

skateman · 2019-11-06T15:56:23Z

ssh, it's green 😉

himdel · 2019-11-25T18:08:29Z

app/javascript/packs/globals.js

 require('jquery.hotkeys');

+// all of patternfly js except for vertical navigation (patternfly-functions-vertical-nav.js)


so, this bit didn't really work,
because require('angular-patternfly') requires the whole thing anyway.

Fixing in #6449,
and removing the call to setupVerticalNavigation which no longer works without patternfly-functions-vertical-nav.js.

Hyperkid123 reviewed Jul 23, 2019

View reviewed changes

martinpovolny changed the title ~~Vertical navbar~~ [WIP] Vertical navbar Jul 23, 2019

Hyperkid123 reviewed Jul 23, 2019

View reviewed changes

miq-bot added react hammer/no wip labels Jul 23, 2019

martinpovolny added the ivanchuk/no label Jul 24, 2019

miq-bot requested a review from epwinchell July 29, 2019 18:00

epwinchell approved these changes Jul 29, 2019

View reviewed changes

martinpovolny mentioned this pull request Jul 30, 2019

[WIP] React menubar #5610

Closed

epwinchell mentioned this pull request Jul 30, 2019

Vertical Nav responsive state updates #5907

Closed

Hyperkid123 approved these changes Jul 31, 2019

View reviewed changes

fhlavac force-pushed the react_menubar branch 2 times, most recently from 4c12d7e to a8df32e Compare August 27, 2019 08:58

fhlavac changed the title ~~[WIP] Vertical navbar~~ Vertical navbar Aug 27, 2019

fhlavac added 10 commits November 6, 2019 12:31

Cleaned code and updated snapshot

1da7629

Addded missing # to data-targets

805ab47

Updated snapshot

717e6eb

Removed PF js imports for vertical navbar

fc71dab

Replaced list-group-item class with custom class

7a3ff1e

Replaced onMouseDown handling with onClick

595afdb

Updated third-level section test and snapshot

e088660

Added "collapse" styling at the top-level

74903ab

Updated test snapshots

53897c9

Added id to top and second level items

9dc4894

fhlavac force-pushed the react_menubar branch from 45e559a to 9dc4894 Compare November 6, 2019 11:35

fhlavac added 3 commits November 6, 2019 13:19

Remove expanding at top-level when no items

72d108a

This adds non expanging markup for top-level items with no child items

Update snapshots after adding top+second level ids

bd79296

Update dashboard_toolbar snapshot

4a288aa

skateman approved these changes Nov 6, 2019

View reviewed changes

fhlavac added 3 commits November 6, 2019 16:20

Make IDs for sections and items different

1a07efd

Remove data-targets at top and second level

a727a4b

Update main-menu snapshots

4eb3f7d

himdel merged commit e0131a4 into ManageIQ:master Nov 6, 2019

himdel added this to the Sprint 124 Ending Nov 11, 2019 milestone Nov 6, 2019

himdel mentioned this pull request Nov 6, 2019

Refactor get_rec_cls to get proper record class for various items #6224

Merged

himdel reviewed Nov 25, 2019

View reviewed changes

himdel mentioned this pull request Feb 26, 2020

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

Merged

h-kataria mentioned this pull request Feb 22, 2021

menu_section - point to specific menu item #892

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vertical navbar #5854

Vertical navbar #5854

fhlavac commented Jul 23, 2019 •

edited

Loading

Hyperkid123 commented Jul 23, 2019

Hyperkid123 Jul 23, 2019

Hyperkid123 Jul 23, 2019

Hyperkid123 Jul 23, 2019

martinpovolny commented Jul 23, 2019

Hyperkid123 Jul 23, 2019

Hyperkid123 commented Jul 23, 2019

epwinchell commented Jul 29, 2019

epwinchell left a comment

epwinchell commented Jul 29, 2019

epwinchell commented Jul 29, 2019

Hyperkid123 left a comment

martinpovolny commented Jul 31, 2019

himdel commented Jul 31, 2019

himdel commented Jul 31, 2019 •

edited

Loading

martinpovolny commented Jul 31, 2019

himdel commented Jul 31, 2019

epwinchell commented Jul 31, 2019

fhlavac commented Aug 27, 2019

skateman commented Nov 6, 2019

skateman left a comment

himdel commented Nov 6, 2019 •

edited

Loading

miq-bot commented Nov 6, 2019

fhlavac commented Nov 6, 2019 •

edited

Loading

himdel commented Nov 6, 2019

skateman commented Nov 6, 2019

himdel Nov 25, 2019

		require('jquery.hotkeys');

		// all of patternfly js except for vertical navigation (patternfly-functions-vertical-nav.js)

Vertical navbar #5854

Vertical navbar #5854

Conversation

fhlavac commented Jul 23, 2019 • edited Loading

Task

Hyperkid123 commented Jul 23, 2019

Hyperkid123 Jul 23, 2019

Choose a reason for hiding this comment

Hyperkid123 Jul 23, 2019

Choose a reason for hiding this comment

Hyperkid123 Jul 23, 2019

Choose a reason for hiding this comment

martinpovolny commented Jul 23, 2019

Hyperkid123 Jul 23, 2019

Choose a reason for hiding this comment

Hyperkid123 commented Jul 23, 2019

epwinchell commented Jul 29, 2019

epwinchell left a comment

Choose a reason for hiding this comment

epwinchell commented Jul 29, 2019

epwinchell commented Jul 29, 2019

Hyperkid123 left a comment

Choose a reason for hiding this comment

martinpovolny commented Jul 31, 2019

himdel commented Jul 31, 2019

himdel commented Jul 31, 2019 • edited Loading

martinpovolny commented Jul 31, 2019

himdel commented Jul 31, 2019

epwinchell commented Jul 31, 2019

fhlavac commented Aug 27, 2019

skateman commented Nov 6, 2019

skateman left a comment

Choose a reason for hiding this comment

himdel commented Nov 6, 2019 • edited Loading

miq-bot commented Nov 6, 2019

fhlavac commented Nov 6, 2019 • edited Loading

himdel commented Nov 6, 2019

skateman commented Nov 6, 2019

himdel Nov 25, 2019

Choose a reason for hiding this comment

fhlavac commented Jul 23, 2019 •

edited

Loading

himdel commented Jul 31, 2019 •

edited

Loading

himdel commented Nov 6, 2019 •

edited

Loading

fhlavac commented Nov 6, 2019 •

edited

Loading