From bfe3a4b1b144cf6617e829a58cf53c13e37fd8dc Mon Sep 17 00:00:00 2001 From: Martin Hradil Date: Thu, 9 Apr 2020 01:55:13 +0000 Subject: [PATCH 1/7] miq menus - unify link props by menu item type handling both navbar and main menu use the same kind of menu items, with the same set of types getTargetByType was shared, but it looks like all the places used the same logic for href, target & onClick (the onClick logic *may* differ in return true/false/undefined now) --- .../components/main-menu/helpers.js | 18 ------------ .../components/main-menu/second-level.jsx | 17 ++--------- .../components/main-menu/third-level.jsx | 14 ++------- .../components/main-menu/top-level.jsx | 24 +++------------ app/javascript/components/top-navbar/help.jsx | 7 ++--- .../components/top-navbar/helpers.js | 4 --- app/javascript/helpers/get-target-by-type.js | 3 -- app/javascript/menu/item-type.js | 29 +++++++++++++++++++ 8 files changed, 40 insertions(+), 76 deletions(-) delete mode 100644 app/javascript/components/top-navbar/helpers.js delete mode 100644 app/javascript/helpers/get-target-by-type.js create mode 100644 app/javascript/menu/item-type.js diff --git a/app/javascript/components/main-menu/helpers.js b/app/javascript/components/main-menu/helpers.js index 8aae44f65ef..391fdcb8587 100644 --- a/app/javascript/components/main-menu/helpers.js +++ b/app/javascript/components/main-menu/helpers.js @@ -1,27 +1,9 @@ -export const getHrefByType = (type, href, id) => { - switch (type) { - case 'big_iframe': - return `/dashboard/iframe?id=${id}`; - case 'modal': - return undefined; - default: - return href; - } -}; - export const getItemId = id => `menu_item_${id}`; export const getSectionId = id => `menu_section_${id}`; export const getIdByCategory = (isSection, id) => (isSection ? getSectionId(id) : getItemId(id)); -export const handleUnsavedChanges = (type) => { - if (type === 'modal') { - return sendDataWithRx({ type: 'showAboutModal' }); - } - return window.miqCheckForChanges(); -}; - export const saveVerticalMenuState = (isVerticalMenuCollapsed) => { window.localStorage.setItem('patternfly-navigation-primary', isVerticalMenuCollapsed ? 'collapsed' : 'expanded'); }; diff --git a/app/javascript/components/main-menu/second-level.jsx b/app/javascript/components/main-menu/second-level.jsx index d2c51759231..8c0b6c98d13 100644 --- a/app/javascript/components/main-menu/second-level.jsx +++ b/app/javascript/components/main-menu/second-level.jsx @@ -3,10 +3,8 @@ import PropTypes from 'prop-types'; import ClassNames from 'classnames'; import { MenuItem, HoverContext } from './main-menu'; import { menuProps } from './recursive-props'; -import { - getHrefByType, getIdByCategory, handleUnsavedChanges, -} from './helpers'; -import getTargetByType from '../../helpers/get-target-by-type'; +import { getIdByCategory } from './helpers'; +import { linkProps } from '../../menu/item-type'; const SecondLevel = ({ id, @@ -35,16 +33,7 @@ const SecondLevel = ({ onMouseEnter={() => (handleSetActiveIds(hasSubitems ? { secondLevelId: id } : undefined))} onMouseLeave={() => handleSetActiveIds({ secondLevelId: undefined })} > - { - if (handleUnsavedChanges(type) === false) { - event.preventDefault(); - } - return false; - }} - target={getTargetByType(type)} - > + {title}
diff --git a/app/javascript/components/main-menu/third-level.jsx b/app/javascript/components/main-menu/third-level.jsx index a35fba1f5b9..d5f1f40db17 100644 --- a/app/javascript/components/main-menu/third-level.jsx +++ b/app/javascript/components/main-menu/third-level.jsx @@ -1,8 +1,7 @@ import React from 'react'; import ClassNames from 'classnames'; import { menuProps } from './recursive-props'; -import { getHrefByType, handleUnsavedChanges } from './helpers'; -import getTargetByType from '../../helpers/get-target-by-type'; +import { linkProps } from '../../menu/item-type'; const ThirdLevel = ({ id, @@ -21,16 +20,7 @@ const ThirdLevel = ({ }, )} > - { - if (handleUnsavedChanges(type) === false) { - event.preventDefault(); - } - return false; - }} - target={getTargetByType(type)} - > + {title} diff --git a/app/javascript/components/main-menu/top-level.jsx b/app/javascript/components/main-menu/top-level.jsx index 4401db7d9ac..fe3640e6851 100644 --- a/app/javascript/components/main-menu/top-level.jsx +++ b/app/javascript/components/main-menu/top-level.jsx @@ -3,10 +3,8 @@ import PropTypes from 'prop-types'; import ClassNames from 'classnames'; import { MenuItem, HoverContext } from './main-menu'; import { menuProps, RecursiveMenuProps } from './recursive-props'; -import { - getHrefByType, getSectionId, handleUnsavedChanges, getItemId, -} from './helpers'; -import getTargetByType from '../../helpers/get-target-by-type'; +import { getSectionId, getItemId } from './helpers'; +import { linkProps } from '../../menu/item-type'; const TopLevel = ({ level, @@ -38,14 +36,7 @@ const TopLevel = ({ > { - if (handleUnsavedChanges(type) === false) { - event.preventDefault(); - } - return false; - }} - target={getTargetByType(type)} + {...linkProps({ type, href, id })} > {title} @@ -81,14 +72,7 @@ const TopLevel = ({ > { - if (handleUnsavedChanges(type) === false) { - event.preventDefault(); - } - return false; - }} - target={getTargetByType(type)} + {...linkProps({ type, href, id })} > {title} diff --git a/app/javascript/components/top-navbar/help.jsx b/app/javascript/components/top-navbar/help.jsx index 00b8577eb98..b45bab78865 100644 --- a/app/javascript/components/top-navbar/help.jsx +++ b/app/javascript/components/top-navbar/help.jsx @@ -1,9 +1,8 @@ import React from 'react'; import { Dropdown, Icon, MenuItem } from 'patternfly-react'; import PropTypes from 'prop-types'; -import { showAboutModal } from './helpers'; import { helpMenuProps, recursiveHelpMenuProps } from './recursive-props'; -import getTargetByType from '../../helpers/get-target-by-type'; +import { linkProps } from '../../menu/item-type'; const Help = ({ helpMenu, @@ -47,9 +46,7 @@ const Help = ({ (item.type === 'modal' ? showAboutModal(e) : !miqCheckForChanges() && e.preventDefault())} - target={getTargetByType(item.type)} + {...linkProps(item)} > {item.title} diff --git a/app/javascript/components/top-navbar/helpers.js b/app/javascript/components/top-navbar/helpers.js deleted file mode 100644 index b28f9ec8ebd..00000000000 --- a/app/javascript/components/top-navbar/helpers.js +++ /dev/null @@ -1,4 +0,0 @@ -export const showAboutModal = (e) => { - e.preventDefault(); - return sendDataWithRx({ type: 'showAboutModal' }); -}; diff --git a/app/javascript/helpers/get-target-by-type.js b/app/javascript/helpers/get-target-by-type.js deleted file mode 100644 index ebe6fbc02b7..00000000000 --- a/app/javascript/helpers/get-target-by-type.js +++ /dev/null @@ -1,3 +0,0 @@ -const getTargetByType = type => (type === 'new_window' ? '_blank' : '_self'); - -export default getTargetByType; diff --git a/app/javascript/menu/item-type.js b/app/javascript/menu/item-type.js new file mode 100644 index 00000000000..638f8e3496a --- /dev/null +++ b/app/javascript/menu/item-type.js @@ -0,0 +1,29 @@ +// there are 4 menu item types (used both in navbar and menu) +// * default (href) - opens href +// * big_iframe (id) - no menu, only navbar and ..a big iframe (external with our header) +// * modal () - open the About Modal (extend for any modals) +// * new_window (href) - opens href in new window (for external links) + +export const linkProps = ({type, href, id}) => ({ + href: { + big_iframe: `/dashboard/iframe?id=${id}`, + default: href, + modal: undefined, + new_window: href, + }[type], + + target: (type === 'new_window' ? '_blank' : '_self'), + + onClick: (event) => { + if (type === 'modal') { + sendDataWithRx({ type: 'showAboutModal' }); + event.preventDefault(); + return; + } + + if (['default', 'big_iframe'].includes(type) && miqCheckForChanges() === false) { + event.preventDefault(); + return; + } + }, +}); From 912453901dc2150f736726f4d483d5c29ce11472 Mon Sep 17 00:00:00 2001 From: Martin Hradil Date: Thu, 9 Apr 2020 02:13:51 +0000 Subject: [PATCH 2/7] menu helper - item id, use for third level too --- app/javascript/components/main-menu/second-level.jsx | 5 ++--- app/javascript/components/main-menu/third-level.jsx | 4 ++-- app/javascript/components/main-menu/top-level.jsx | 7 +++---- app/javascript/menu/item-type.js | 2 ++ 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/app/javascript/components/main-menu/second-level.jsx b/app/javascript/components/main-menu/second-level.jsx index 8c0b6c98d13..fa49533dfe7 100644 --- a/app/javascript/components/main-menu/second-level.jsx +++ b/app/javascript/components/main-menu/second-level.jsx @@ -3,8 +3,7 @@ import PropTypes from 'prop-types'; import ClassNames from 'classnames'; import { MenuItem, HoverContext } from './main-menu'; import { menuProps } from './recursive-props'; -import { getIdByCategory } from './helpers'; -import { linkProps } from '../../menu/item-type'; +import { itemId, linkProps } from '../../menu/item-type'; const SecondLevel = ({ id, @@ -29,7 +28,7 @@ const SecondLevel = ({ active, }, )} - id={getIdByCategory(hasSubitems, id)} + id={itemId(id, hasSubitems)} onMouseEnter={() => (handleSetActiveIds(hasSubitems ? { secondLevelId: id } : undefined))} onMouseLeave={() => handleSetActiveIds({ secondLevelId: undefined })} > diff --git a/app/javascript/components/main-menu/third-level.jsx b/app/javascript/components/main-menu/third-level.jsx index d5f1f40db17..a8c4f339d97 100644 --- a/app/javascript/components/main-menu/third-level.jsx +++ b/app/javascript/components/main-menu/third-level.jsx @@ -1,7 +1,7 @@ import React from 'react'; import ClassNames from 'classnames'; import { menuProps } from './recursive-props'; -import { linkProps } from '../../menu/item-type'; +import { itemId, linkProps } from '../../menu/item-type'; const ThirdLevel = ({ id, @@ -12,7 +12,7 @@ const ThirdLevel = ({ type, }) => (!visible ? null : (
  • handleSetActiveIds({ topLevelId: id })} onBlur={() => undefined} > @@ -59,7 +58,7 @@ const TopLevel = ({ return (
  • ({ } }, }); + +export const itemId = (id, section) => (section ? `menu_section_${id}` : `menu_item_${id}`); From cabc1d347c4302ea43ea54fc3cf798309610c45a Mon Sep 17 00:00:00 2001 From: Martin Hradil Date: Thu, 9 Apr 2020 02:14:34 +0000 Subject: [PATCH 3/7] split helper into places where the 2 independent functions are used --- .../components/main-menu/helpers.js | 20 ------------------- .../components/main-menu/main-menu.jsx | 12 +++++++++-- .../components/top-navbar/navbar-header.jsx | 3 +-- 3 files changed, 11 insertions(+), 24 deletions(-) delete mode 100644 app/javascript/components/main-menu/helpers.js diff --git a/app/javascript/components/main-menu/helpers.js b/app/javascript/components/main-menu/helpers.js deleted file mode 100644 index 391fdcb8587..00000000000 --- a/app/javascript/components/main-menu/helpers.js +++ /dev/null @@ -1,20 +0,0 @@ -export const getItemId = id => `menu_item_${id}`; - -export const getSectionId = id => `menu_section_${id}`; - -export const getIdByCategory = (isSection, id) => (isSection ? getSectionId(id) : getItemId(id)); - -export const saveVerticalMenuState = (isVerticalMenuCollapsed) => { - window.localStorage.setItem('patternfly-navigation-primary', isVerticalMenuCollapsed ? 'collapsed' : 'expanded'); -}; - -export const adaptContentWidth = (isVerticalMenuCollapsed) => { - const content = window.document.getElementsByClassName('container-pf-nav-pf-vertical-with-sub-menus')[0]; - if (content) { - if (isVerticalMenuCollapsed) { - content.classList.add('collapsed-nav'); - } else { - content.classList.remove('collapsed-nav'); - } - } -}; diff --git a/app/javascript/components/main-menu/main-menu.jsx b/app/javascript/components/main-menu/main-menu.jsx index 7d1afd3430a..c765e592bc4 100644 --- a/app/javascript/components/main-menu/main-menu.jsx +++ b/app/javascript/components/main-menu/main-menu.jsx @@ -8,7 +8,6 @@ import TopLevel from './top-level'; import SecondLevel from './second-level'; import ThirdLevel from './third-level'; import { menuProps, RecursiveMenuProps } from './recursive-props'; -import { adaptContentWidth } from './helpers'; const Fallback = props => ; @@ -26,7 +25,16 @@ const MainMenu = ({ menu }) => { const isVerticalMenuCollapsed = useSelector(({ menuReducer: { isVerticalMenuCollapsed } }) => isVerticalMenuCollapsed); useEffect(() => { - adaptContentWidth(isVerticalMenuCollapsed); + const content = document.querySelector('.container-pf-nav-pf-vertical-with-sub-menus'); + if (! content) { + return; + } + + if (isVerticalMenuCollapsed) { + content.classList.add('collapsed-nav'); + } else { + content.classList.remove('collapsed-nav'); + } }, [isVerticalMenuCollapsed]); const handleSetActiveIds = (value) => { diff --git a/app/javascript/components/top-navbar/navbar-header.jsx b/app/javascript/components/top-navbar/navbar-header.jsx index 42ba02fdb56..9818e08e7dc 100644 --- a/app/javascript/components/top-navbar/navbar-header.jsx +++ b/app/javascript/components/top-navbar/navbar-header.jsx @@ -1,7 +1,6 @@ import React, { useEffect } from 'react'; import PropTypes from 'prop-types'; import { useDispatch, useSelector } from 'react-redux'; -import { saveVerticalMenuState } from '../main-menu/helpers'; import { toggleVerticalMenuCollapsed } from '../../miq-redux/menu-reducer'; const NavbarHeader = ({ @@ -11,7 +10,7 @@ const NavbarHeader = ({ const dispatch = useDispatch(); const isVerticalMenuCollapsed = useSelector(({ menuReducer: { isVerticalMenuCollapsed } }) => isVerticalMenuCollapsed); useEffect(() => { - saveVerticalMenuState(isVerticalMenuCollapsed); + window.localStorage.setItem('patternfly-navigation-primary', isVerticalMenuCollapsed ? 'collapsed' : 'expanded'); }, [isVerticalMenuCollapsed]); return ( From 1adbfbdda071f4f4402603a862d4a3323c6c5392 Mon Sep 17 00:00:00 2001 From: Martin Hradil Date: Tue, 5 May 2020 17:48:23 +0000 Subject: [PATCH 4/7] menu - handle logoutInProgress --- app/javascript/menu/item-type.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/javascript/menu/item-type.js b/app/javascript/menu/item-type.js index e9f5c1ca090..2ba3e17d0df 100644 --- a/app/javascript/menu/item-type.js +++ b/app/javascript/menu/item-type.js @@ -25,6 +25,10 @@ export const linkProps = ({type, href, id}) => ({ event.preventDefault(); return; } + + if (href === '/dashboard/logout') { + ManageIQ.logoutInProgress = true; + } }, }); From d54c807d0d49fc593a6f692ef7c5e844a2fff2de Mon Sep 17 00:00:00 2001 From: Martin Hradil Date: Thu, 7 May 2020 21:48:59 +0000 Subject: [PATCH 5/7] item-type: default to "default" for href ensuring logout actually works --- app/javascript/menu/item-type.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/javascript/menu/item-type.js b/app/javascript/menu/item-type.js index 2ba3e17d0df..25d0b8ee557 100644 --- a/app/javascript/menu/item-type.js +++ b/app/javascript/menu/item-type.js @@ -10,7 +10,7 @@ export const linkProps = ({type, href, id}) => ({ default: href, modal: undefined, new_window: href, - }[type], + }[type || 'default'], target: (type === 'new_window' ? '_blank' : '_self'), From ae8096627a861e14b7ad7cb8a5172c09a3cdd72e Mon Sep 17 00:00:00 2001 From: Martin Hradil Date: Fri, 22 May 2020 00:19:06 +0000 Subject: [PATCH 6/7] Drop link_params, use href Menu::Section overrides href for now, but will be removed once we remove clickable sections --- app/helpers/application_helper/navbar.rb | 2 +- app/presenters/menu/item.rb | 11 ----------- app/presenters/menu/section.rb | 15 ++++++++------- 3 files changed, 9 insertions(+), 19 deletions(-) diff --git a/app/helpers/application_helper/navbar.rb b/app/helpers/application_helper/navbar.rb index c6fd462e059..540695e51a4 100644 --- a/app/helpers/application_helper/navbar.rb +++ b/app/helpers/application_helper/navbar.rb @@ -15,7 +15,7 @@ def item_to_hash(item) :id => item.id.to_s, :title => item.name, :icon => item.icon, - :href => item.link_params[:href], + :href => item.href, :type => item.type, :visible => item.visible?, :active => item_active?(item), diff --git a/app/presenters/menu/item.rb b/app/presenters/menu/item.rb index 82d5e3140ba..4708140df11 100644 --- a/app/presenters/menu/item.rb +++ b/app/presenters/menu/item.rb @@ -36,17 +36,6 @@ def visible? ApplicationHelper.role_allows?(rbac_feature) end - def link_params - params = case type.try(:to_sym) - when :big_iframe then {:href => "/dashboard/iframe?id=#{id}"} - when :new_window then {:href => href, :target => '_new'} - when :modal then {:onclick => "sendDataWithRx({type: 'showAboutModal'});", :href => 'javascript:void(0);'} - else {:href => href} - end - params[:onclick] = 'return miqCheckForChanges();' unless type.try(:to_sym) == :modal - params - end - def leaf? true end diff --git a/app/presenters/menu/section.rb b/app/presenters/menu/section.rb index 7a9ca97b8b6..20128962489 100644 --- a/app/presenters/menu/section.rb +++ b/app/presenters/menu/section.rb @@ -42,12 +42,13 @@ def subsection? @subsection ||= Array(items).detect { |el| el.kind_of?(Section) } end - def link_params - params = case type - when :big_iframe then {:href => "/dashboard/iframe?sid=#{id}"} - else {:href => "/dashboard/maintab/?tab=#{id}"} - end - params.merge(:onclick => 'return miqCheckForChanges();') + def href + case type + when :big_iframe + "/dashboard/iframe?sid=#{id}" + else + "/dashboard/maintab/?tab=#{id}" + end end def leaf? @@ -73,7 +74,7 @@ def default_redirect_url items.each do |item| next unless item.visible? if item.kind_of?(Item) - return item.link_params[:href] + return item.href else section_result = item.default_redirect_url return section_result if section_result From ef70489acc5b488adc9854f48bcce9971294a349 Mon Sep 17 00:00:00 2001 From: Martin Hradil Date: Fri, 22 May 2020 00:21:27 +0000 Subject: [PATCH 7/7] TopNavbar snapshot - remove empty href --- .../spec/top-navbar/__snapshots__/top-navbar.spec.js.snap | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/javascript/spec/top-navbar/__snapshots__/top-navbar.spec.js.snap b/app/javascript/spec/top-navbar/__snapshots__/top-navbar.spec.js.snap index 5afb3ec1ee3..0aed93c86c3 100644 --- a/app/javascript/spec/top-navbar/__snapshots__/top-navbar.spec.js.snap +++ b/app/javascript/spec/top-navbar/__snapshots__/top-navbar.spec.js.snap @@ -325,7 +325,6 @@ exports[`Top navbar tests should render correctly 1`] = ` disabled={false} divider={false} header={false} - href="" id="help-menu-about" key=".$about" onClick={[Function]} @@ -339,7 +338,6 @@ exports[`Top navbar tests should render correctly 1`] = ` >