Refs #30344 - review fixes

theforeman · May 15, 2023 · 1411e05 · 1411e05
1 parent a27fb9a
commit 1411e05
Show file tree

Hide file tree

Showing 11 changed files with 80 additions and 79 deletions.
diff --git a/app/assets/stylesheets/base.scss b/app/assets/stylesheets/base.scss
@@ -13,14 +13,23 @@ html {
   overflow: auto;
   height: calc(100% - 70px);
   margin-left: 250px;
+  .rails-table-toolbar {
+    padding-bottom: 0;
+    display: flex;
+    flex-wrap: wrap;
+    justify-content: space-between;
+  }
   &.pf-c-page {
     display: block;
     grid-template-columns: unset;
     grid-template-areas: unset;
   }
+
 }
 
 body {
+  position: absolute; // needed for firefox 100% height bug
+  overflow: auto;
   height: 100%;
   width: 100%;
 }
@@ -170,15 +179,14 @@ select {
   padding-bottom: 9px;
 }
 
-.title_filter_parent {
-  overflow: hidden; // so that the parent will have a height
-  padding-bottom: 0;
-  .title_filter {
-    padding-left: 0;
-  }
+.title_filter {
+  flex-grow: 1;
+  padding-left: 0;
 }
 
 #title_action {
+  flex-grow: 1;
+  display: flex;
   padding-left: 8px;
   padding-bottom: 12px;
 }

diff --git a/app/assets/stylesheets/navigation.scss b/app/assets/stylesheets/navigation.scss
@@ -4,11 +4,6 @@
   .nav a {
     font-size: 13px;
   }
-
-  .user-icon {
-    margin-right: 10px;
-  }
-
   .active > a,
   .open > a {
     background: linear-gradient(
@@ -25,4 +20,7 @@
 
 .pf-c-masthead {
   background: var(--pf-c-masthead--BackgroundColor) image-url('navbar.png');
+  .user-icon {
+    margin-right: 10px;
+  }
 }
diff --git a/app/assets/stylesheets/patternfly_and_overrides.scss b/app/assets/stylesheets/patternfly_and_overrides.scss
@@ -132,6 +132,7 @@ a {
 .btn-toolbar {
   display: flex;
   align-items: center;
+  margin-left: auto;
 
   #toolbar-spinner {
     margin-right: 5px;

diff --git a/app/views/layouts/_application_content.html.erb b/app/views/layouts/_application_content.html.erb
@@ -1,25 +1,23 @@
 <div id="main">
   <div id="content">
-    <div id="breadcrumb">
-      <section class="pf-c-page__main-breadcrumb">
-        <% if content_for?(:breadcrumbs) %>
-          <%= yield(:breadcrumbs) %>
-        <% else %>
-          <%= mount_breadcrumbs %>
-        <% end %>
-      </section>
-    </div>
+    <section class="pf-c-page__main-breadcrumb">
+      <% if content_for?(:breadcrumbs) %>
+        <%= yield(:breadcrumbs) %>
+      <% else %>
+        <%= mount_breadcrumbs %>
+      <% end %>
+    </section>
     <%= yield(:before_search_bar) %>
-    <div class="pf-c-page__main-section pf-m-light title_filter_parent">
-      <div class="title_filter <%= searchable? ? " col-md-6 " : "col-md-4 " %>">
+    <div class="pf-c-page__main-section pf-m-light rails-table-toolbar">
+      <div class="title_filter">
         <%= mount_search_bar if searchable? %>
-        <%= yield(:search_bar) %>&nbsp;
+        <%= yield(:search_bar) %>
       </div>
-      <div id="title_action" class= <%= searchable? ? "col-md-6" : "col-md-8" %>>
+      <div id="title_action">
         <% if filter_columns? %>
           <div class="pull-left">
             <%= mount_column_selector %>
-            <%= yield(:column_selector) %>&nbsp;
+            <%= yield(:column_selector) %>
           </div>
         <% end %>
         <div class="btn-toolbar pull-right">

diff --git a/test/integration/compute_profile_js_test.rb b/test/integration/compute_profile_js_test.rb
@@ -37,7 +37,7 @@ class ComputeProfileJSTest < IntegrationTestWithJavascript
     # amazon123 exists in fixture compute_attributes.yml
     click_link("amazon123 (eu-west-1-EC2)")
 
-    assert page.has_selector?('#breadcrumb .active', :text => compute_profiles(:one).name), "#{compute_profiles(:one).name} was expected in the breadcrumb active, but was not found"
+    assert page.has_selector?('.pf-c-page__main-breadcrumb .active', :text => compute_profiles(:one).name), "#{compute_profiles(:one).name} was expected in the breadcrumb active, but was not found"
     selected_profile = find("#s2id_compute_attribute_compute_profile_id .select2-chosen").text
     assert_equal compute_profiles(:one).name, selected_profile
 

diff --git a/test/integration_test_helper.rb b/test/integration_test_helper.rb
@@ -55,7 +55,7 @@ def assert_index_page(index_path, title_text, new_link_text = nil, has_search =
   end
 
   def assert_breadcrumb_text(text)
-    assert page.has_selector?(:xpath, "//div[@id='breadcrumb']//*[contains(.,'#{text}')]"), "#{text} was expected in the div[id='breadcrumb'] tag, but was not found"
+    assert page.has_selector?(:xpath, "//section//div[contains(@class, 'breadcrumb-bar') or contains(@id, 'breadcrumb')]  //*[contains(.,'#{text}')]"), "#{text} was expected in //section//div[contains(@class, 'breadcrumb-bar') or contains(@id, 'breadcrumb')], but was not found"
   end
 
   def assert_new_button(index_path, new_link_text, new_path)
@@ -208,7 +208,7 @@ def refute_available_organization(organization)
   def refute_available_organization_menu(organization)
     within('.location-menu') do
       first('button').click
-      within('.location-menu section ul ul', visible: :all) do
+      within('.location-menu section ul', visible: :all) do
         assert page.has_no_link?(organization)
       end
     end

diff --git a/webpack/assets/javascripts/hosts/tableCheckboxes.js b/webpack/assets/javascripts/hosts/tableCheckboxes.js
@@ -279,6 +279,7 @@ function updateCounter() {
 
   item.attr('data-original-title', title);
   item.tooltip({
+    container: 'body',
     trigger: 'hover',
   });
   return false;

diff --git a/webpack/assets/javascripts/react_app/components/BreadcrumbBar/BreadcrumbBar.scss b/webpack/assets/javascripts/react_app/components/BreadcrumbBar/BreadcrumbBar.scss
@@ -2,6 +2,8 @@
   .breadcrumb-line {
     margin-right: -60px;
     margin-left: -60px;
+    margin-top: 0;
+    margin-bottom: 0;
   }
 
   .breadcrumbs-list {

diff --git a/webpack/assets/javascripts/react_app/components/Layout/Navigation.js b/webpack/assets/javascripts/react_app/components/Layout/Navigation.js
@@ -1,12 +1,11 @@
-import React, { useEffect, useRef, useMemo } from 'react';
+import React, { useState, useEffect, useRef, useMemo } from 'react';
 import PropTypes from 'prop-types';
 import classNames from 'classnames';
 import {
   Nav,
   NavList,
   NavItem,
   NavExpandable,
-  NavGroup,
   NavItemSeparator,
 } from '@patternfly/react-core';
 import { getCurrentPath } from './LayoutHelper';
@@ -24,6 +23,7 @@ const Navigation = ({
   setNavigationActiveItem,
 }) => {
   const clearTimerRef = useRef();
+  const [currentPath, setCurrentPath] = useState(window.location.pathname);
   useEffect(
     () => () => {
       if (clearTimerRef.current) clearTimeout(clearTimerRef.current);
@@ -50,10 +50,9 @@ const Navigation = ({
     });
   });
 
-  const getGroupedItems = useMemo(
+  const groupedItems = useMemo(
     () =>
       items.map(({ subItems, ...rest }) => {
-        const { pathname } = window.location;
         const groups = [];
         let currIndex = 0;
         if (subItems.length) {
@@ -69,18 +68,21 @@ const Navigation = ({
             } else {
               groups[currIndex].groupItems.push({
                 ...sub,
-                isActive: pathname === sub.href.split('?')[0],
+                isActive: currentPath === sub.href.split('?')[0],
               });
             }
           });
         }
         return { ...rest, groups };
       }),
     // eslint-disable-next-line react-hooks/exhaustive-deps
-    [items.length]
+    [items.length, currentPath]
   );
 
-  const groupedItems = getGroupedItems;
+  const clickAndNavigate = (_onClick, href) => {
+    _onClick && _onClick();
+    setCurrentPath(href);
+  };
   return (
     <Nav id="foreman-nav">
       <NavList>
@@ -103,36 +105,34 @@ const Navigation = ({
             >
               {groups.map((group, groupIndex) =>
                 groupIndex === 0 ? (
-                  <NavGroup key={0} title={group.title}>
-                    {group.groupItems.map(
-                      (
-                        {
-                          id,
-                          title: subItemTitle,
-                          className: subItemClassName,
-                          href,
-                          onClick,
-                          isActive,
-                        },
-                        groupItemsIndex
-                      ) => (
-                        <React.Fragment key={id}>
-                          <NavItem
-                            className={subItemClassName}
-                            id={id}
-                            to={href}
-                            onClick={onClick}
-                            isActive={isActive}
-                          >
-                            {subItemTitle}
-                          </NavItem>
-                          {groupItemsIndex !== group.groupItems.length - 1 && (
-                            <NavItemSeparator />
-                          )}
-                        </React.Fragment>
-                      )
-                    )}
-                  </NavGroup>
+                  group.groupItems.map(
+                    (
+                      {
+                        id,
+                        title: subItemTitle,
+                        className: subItemClassName,
+                        href,
+                        onClick,
+                        isActive,
+                      },
+                      groupItemsIndex
+                    ) => (
+                      <React.Fragment key={id}>
+                        <NavItem
+                          className={subItemClassName}
+                          id={id}
+                          to={href}
+                          onClick={() => clickAndNavigate(onClick, href)}
+                          isActive={isActive}
+                        >
+                          {subItemTitle}
+                        </NavItem>
+                        {groupItemsIndex !== group.groupItems.length - 1 && (
+                          <NavItemSeparator />
+                        )}
+                      </React.Fragment>
+                    )
+                  )
                 ) : (
                   <React.Fragment key={groupIndex}>
                     <NavItemSeparator />
@@ -159,7 +159,7 @@ const Navigation = ({
                               className={subItemClassName}
                               id={id}
                               to={href}
-                              onClick={onClick}
+                              onClick={() => clickAndNavigate(onClick, href)}
                               isActive={isActive}
                             >
                               {subItemTitle}

diff --git a/webpack/assets/javascripts/react_app/components/Layout/layout.scss b/webpack/assets/javascripts/react_app/components/Layout/layout.scss
@@ -31,6 +31,7 @@
 
 .pf-c-nav {
   .nav-title-icon {
+    pointer-events: none;
     padding-right: 10px;
   }
   .nav-title {
@@ -46,16 +47,9 @@
     // in small screens, the expandable items have css to add a second border
     border-bottom: none;
   }
-}
-
-.pf-c-nav {
-  .nav-title-icon {
-    pointer-events: none;
-    padding-right: 10px;
-  }
-
-  .nav-title {
-    pointer-events: none;
+
+  .pf-m-expandable>.pf-c-nav__link{
+    text-align: left;
   }
 }
 

diff --git a/webpack/assets/javascripts/react_app/routes/common/PageLayout/PageLayout.js b/webpack/assets/javascripts/react_app/routes/common/PageLayout/PageLayout.js
@@ -45,7 +45,7 @@ const PageLayout = ({
     {(searchable || beforeToolbarComponent || isLoading || toolbarButtons) && (
       <PageSection variant={PageSectionVariants.light}>
         {beforeToolbarComponent}
-        <div>
+        <div className="title_filter_parent">
           <Col className="title_filter" md={searchable ? 6 : 4}>
             {searchable && (
               <SearchBar
@@ -56,9 +56,8 @@ const PageLayout = ({
                 onSearch={onSearch}
               />
             )}
-            &nbsp;
           </Col>
-          <Col id="title_action" md={searchable ? 6 : 8}>
+          <Col md={searchable ? 6 : 8}>
             <div className="btn-toolbar pull-right">
               {isLoading && (
                 <div id="toolbar-spinner">