Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Serverless/Chrome] App menu bar fixes #162002

Merged
merged 35 commits into from
Aug 11, 2023

Conversation

tsullivan
Copy link
Member

@tsullivan tsullivan commented Jul 14, 2023

Summary

Closes #161889
Closes #162935

This PR gives the correct look and feel to the app menu bar.

  1. The bar appears to the right of the side panel
  2. The bar has fixed position below the fixed EuiHeader
  3. Page content flows after the the bar

Testing

  1. Run yarn es snapshot in a terminal pane
  2. Run yarn serverless in another pane
  3. Log into the Kibana UI and check different forms of the menu bar
  4. Use the dev-project switcher to test other solution projects
  5. Test with a header banner:
    #!/bin/bash
    HOST=http://elastic:changeme@localhost:5601
    curl -X POST "$HOST/internal/kibana/settings" \
     -H 'kbn-xsrf: true' \
     -H 'X-Elastic-Internal-Origin: Kibana' \
     -H 'Content-Type: application/json' \
     -d '{
       "changes": {
         "banners:placement": "top",
         "banners:textContent": "Prefix. **SIMPLE BANNER MESSAGE CONTENT**. Suffix."
       }
     }'
    
    Set banners:placement to null to turn off the header banner.

Known issue: in some Observability project pages, the app menu bar may appear as an empty div. This is explained in the serverless project layout documentation:

Note The display of the toolbar container is conditional, based on whether there is toolbar content to show. Make sure to pass undefined to setHeaderActionMenu if there is no application content to show. In classic layout, passing an empty span or div element suffices to "clear" the toolbar, but in serverless projects it will cause a large empty container to show below the header.

Screenshots

Will not be updated past a5222e4

Project layout in Observability app https://github.com/elastic/kibana/assets/908371/9fb8f57a-3de9-49e8-9d6d-d10fa83a3c83
Project layout in Observability app w/ header banner https://github.com/elastic/kibana/assets/908371/19a0bf68-0df7-4f08-b987-125abe9e5680
Project layout in Security app https://github.com/elastic/kibana/assets/908371/af1940fa-9d48-48a4-b675-0b3c8bcffc39
Project layout in Security app w/ header banner https://github.com/elastic/kibana/assets/908371/d962952a-1d21-4bb3-8992-cafe4aed82a4

Checklist

Delete any items that are not applicable to this PR.

@tsullivan tsullivan force-pushed the serverless/header-bar-2-ii branch from c7c01ad to 6a3d794 Compare July 14, 2023 23:44
@tsullivan tsullivan changed the title [Serverless/Chrome] Fix second-level header bar to not be visible at all times [Serverless/Chrome] Fix app menu bar to not be visible at all times Jul 17, 2023
@tsullivan
Copy link
Member Author

I'm finding a lot of slowdown when experimenting with style changes in packages. I'm going to pivot to POC work in Storybook to experiment with EuiPageTemplate

@tsullivan tsullivan force-pushed the serverless/header-bar-2-ii branch from 5d81795 to 0ac609d Compare July 18, 2023 19:45
@tsullivan tsullivan added the Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) label Aug 1, 2023
@tsullivan tsullivan self-assigned this Aug 1, 2023
@tsullivan tsullivan force-pushed the serverless/header-bar-2-ii branch from b21d97c to a5222e4 Compare August 2, 2023 00:54
@tsullivan tsullivan changed the title [Serverless/Chrome] Fix app menu bar to not be visible at all times [Serverless/Chrome] App menu bar fixes Aug 2, 2023
@tsullivan tsullivan added the Project:Serverless Work as part of the Serverless project for its initial release label Aug 2, 2023
@tsullivan tsullivan force-pushed the serverless/header-bar-2-ii branch from 4316977 to a5c2189 Compare August 2, 2023 20:12
@tsullivan tsullivan marked this pull request as ready for review August 2, 2023 20:20
@tsullivan tsullivan requested review from a team as code owners August 2, 2023 20:20
@elasticmachine
Copy link
Contributor

Pinging @elastic/appex-sharedux (Team:SharedUX)

@paul-tavares
Copy link
Contributor

Hi @tsullivan ,
I checked out your PR and tested the Response Console page overlay as you requested and its not displaying correctly. It looks like this PR reverts back to showing Flyouts just below the header_firstBar:

Screenshot 2023-08-09 at 9 59 53 AM



To fix this, you just need to revert the change you made the last time (f82588b). See below for a quick git patch you can apply to your PR.

GIT patch with fix

Apply this git patch to fix the issue:

Index: x-pack/plugins/security_solution/public/management/components/page_overlay/page_overlay.tsx
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/x-pack/plugins/security_solution/public/management/components/page_overlay/page_overlay.tsx b/x-pack/plugins/security_solution/public/management/components/page_overlay/page_overlay.tsx
--- a/x-pack/plugins/security_solution/public/management/components/page_overlay/page_overlay.tsx	(revision d0330d2191c987cb00c1aff062b0c7c0834ea2ae)
+++ b/x-pack/plugins/security_solution/public/management/components/page_overlay/page_overlay.tsx	(date 1691589306484)
@@ -89,6 +89,17 @@
   body.${PAGE_OVERLAY_DOCUMENT_BODY_FULLSCREEN_CLASSNAME} {
     ${FULL_SCREEN_CONTENT_OVERRIDES_CSS_STYLESHEET}
   }
+
+  //-------------------------------------------------------------------------------------------
+  // Style overrides for when Page Overlay is displayed in serverless project
+  //-------------------------------------------------------------------------------------------
+  // With serverless, there is 1 less header displayed, thus the display of the page overlay
+  // need to be adjusted slightly so that it still display below the header
+  //-------------------------------------------------------------------------------------------
+  body.kbnBody.kbnBody--projectLayout:not(.${PAGE_OVERLAY_DOCUMENT_BODY_FULLSCREEN_CLASSNAME}) .${PAGE_OVERLAY_CSS_CLASSNAME} {
+    top: ${({ theme: { eui } }) => eui.euiHeaderHeightCompensation};
+    height: calc(100% - (${({ theme: { eui } }) => eui.euiHeaderHeightCompensation}));
+  }
 `;
 
 const setDocumentBodyOverlayIsVisible = () => {

Let me know if you have any questions.

Copy link
Contributor

@semd semd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested the Security side navigation locally. Everything looks great, the positioning has been fixed and all parts fit correctly in the layout. LGTM! 👍

@tsullivan tsullivan requested a review from a team as a code owner August 9, 2023 17:32
@tsullivan
Copy link
Member Author

I checked out your PR and tested the Response Console page overlay as you requested and its not displaying correctly.

@paul-tavares Thanks for the guidance! I have pushed ac44334. Apologies, but I am still not sure how to get a screenshot and show the result. Would you be able to take another look?

Copy link
Contributor

@paul-tavares paul-tavares left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Security Solution Page Overlay component looks good now. Thank you.

@tsullivan
Copy link
Member Author

tsullivan commented Aug 9, 2023

@cee-chen @MichaelMarcialis

I'm personally ambivalent as to whether the app menu bar should have a fixed or flexible height. Dev-wise, a fixed height is easier, but design-wise, there may be reasons for a variable height. @MichaelMarcialis would be able to say for sure.

I took another wack at using position: sticky for the app menu bar, which wouldn't require a fixed height. I had trouble using this style earlier but it's going way better now, likely thanks to 1b2423c and/or 2acd034. It's just not quite perfect - there is some wiggle in the app menu bar that happens with scrolling while hovering over the element:

app.meny.bar.sticky.mp4
Here was the change I was experimenting with
diff --git a/packages/core/chrome/core-chrome-browser-internal/src/ui/project/app_menu.tsx b/packages/core/chrome/core-chrome-browser-internal/src/ui/project/app_menu.tsx
index 45fa1aec2eb..6aad187cc0a 100644
--- a/packages/core/chrome/core-chrome-browser-internal/src/ui/project/app_menu.tsx
+++ b/packages/core/chrome/core-chrome-browser-internal/src/ui/project/app_menu.tsx
@@ -11,13 +11,12 @@ import { css } from '@emotion/react';
 import { MountPoint } from '@kbn/core-mount-utils-browser';
 import React from 'react';
 import { HeaderActionMenu } from '../header/header_action_menu';
-import { SIZE_COLLAPSED, SIZE_EXPANDED } from './navigation';
 
 interface AppMenuBarProps {
   isOpen: boolean;
   headerActionMenuMounter: { mount: MountPoint<HTMLElement> | undefined };
 }
-export const AppMenuBar = ({ isOpen, headerActionMenuMounter }: AppMenuBarProps) => {
+export const AppMenuBar = ({ headerActionMenuMounter }: AppMenuBarProps) => {
   const { euiTheme } = useEuiTheme();
   return (
     <EuiThemeProvider>
@@ -31,8 +30,6 @@ export const AppMenuBar = ({ isOpen, headerActionMenuMounter }: AppMenuBarProps)
           justify-content: end;
           align-items: center;
           padding: ${euiTheme.size.s};
-          /* span the width of the body content (viewport - width of side nav) */
-          width: calc(100% - ${isOpen ? SIZE_EXPANDED : SIZE_COLLAPSED}px);
         `}
       >
         <HeaderActionMenu mounter={headerActionMenuMounter} />
diff --git a/packages/core/chrome/core-chrome-browser-internal/src/ui/project/header.tsx b/packages/core/chrome/core-chrome-browser-internal/src/ui/project/header.tsx
index de7c42e6b41..d8c061b26f4 100644
--- a/packages/core/chrome/core-chrome-browser-internal/src/ui/project/header.tsx
+++ b/packages/core/chrome/core-chrome-browser-internal/src/ui/project/header.tsx
@@ -189,13 +189,7 @@ export const ProjectHeader = ({
       <SkipToMainContent />
 
       <HeaderTopBanner headerBanner$={observables.headerBanner$} />
-      <header
-        data-test-subj="kibanaProjectHeader"
-        css={css`
-          /* FIXME use a variable for the app bar height */
-          margin-bottom: ${headerActionMenuMounter.mount ? 48 : 0}px;
-        `}
-      >
+      <header data-test-subj="kibanaProjectHeader">
         <div id="globalHeaderBars" data-test-subj="headerGlobalNav" className="header__bars">
           <EuiHeader position="fixed" className="header__firstBar">
             <EuiHeaderSection grow={false}>
diff --git a/packages/core/chrome/core-chrome-browser-internal/src/ui/project/navigation.tsx b/packages/core/chrome/core-chrome-browser-internal/src/ui/project/navigation.tsx
index 9e07c1154d2..c05e8c3c4b9 100644
--- a/packages/core/chrome/core-chrome-browser-internal/src/ui/project/navigation.tsx
+++ b/packages/core/chrome/core-chrome-browser-internal/src/ui/project/navigation.tsx
@@ -10,8 +10,8 @@ import React from 'react';
 import { css } from '@emotion/react';
 import { EuiCollapsibleNav, EuiCollapsibleNavProps, useIsWithinMinBreakpoint } from '@elastic/eui';
 
-export const SIZE_EXPANDED = 248;
-export const SIZE_COLLAPSED = 48;
+const SIZE_EXPANDED = 248;
+const SIZE_COLLAPSED = 48;
 
 export interface ProjectNavigationProps {
   isOpen: boolean;
diff --git a/src/core/public/styles/chrome/_project.scss b/src/core/public/styles/chrome/_project.scss
index 623a83fce60..a9c014b0de0 100644
--- a/src/core/public/styles/chrome/_project.scss
+++ b/src/core/public/styles/chrome/_project.scss
@@ -1,10 +1,9 @@
 .kbnBody--projectLayout {
   .header__actionMenu {
     /* fixates the elements position in the viewport, removes the element from the flow of the page */
-    position: fixed;
+    position: sticky;
     /* position below the primary fixed EuiHeader in the viewport */
     top: $euiHeaderHeightCompensation;
-    height: $euiHeaderHeightCompensation;
     /* use a high z-index since this component is part of the header */
     z-index: $euiZHeader;
   }

@tsullivan tsullivan removed the request for review from patrykkopycinski August 10, 2023 15:41
@tsullivan
Copy link
Member Author

@patrykkopycinski I've removed you as a reviewer since Paul T. helped out. Thanks!

/* fixates the elements position in the viewport, removes the element from the flow of the page */
position: sticky;
/* position below the primary fixed EuiHeader in the viewport */
top: 48px;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOTE: in the near future, there will be a variable that can replace this hardcoded 48px

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
core 474 475 +1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 15.6MB 15.6MB +960.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
core 373.3KB 378.2KB +4.9KB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @tsullivan

Copy link
Contributor

@ryankeairns ryankeairns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed my nits, thanks!

@tsullivan tsullivan merged commit 2465550 into elastic:main Aug 11, 2023
@tsullivan tsullivan deleted the serverless/header-bar-2-ii branch August 11, 2023 02:45
@kibanamachine kibanamachine added v8.10.0 backport:skip This commit does not require backporting labels Aug 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Project:Serverless Work as part of the Serverless project for its initial release release_note:skip Skip the PR/issue when compiling release notes Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) v8.10.0
Projects
None yet