Add setHeaderActionMenu API to AppMountParameters #75422

pgayvallet · 2020-08-19T12:39:26Z

Summary

Part of #68524
Related to #72331

Add AppMountParameters.setHeaderActionMenu to allow application to register header application menu
Expose currentActionMenu$ from application service internal start contract

Note: actual consumption of currentActionMenu will be done in #72331, as the new header in not present on master.

Example

export renderApp = ({ element, history, setHeaderActionMenu }: AppMountParameters) => {
   const { renderApp } = await import('./application');
   const { renderActionMenu } = await import('./action_menu');
   setHeaderActionMenu((element) => {
     return renderActionMenu(element);
   })
   return renderApp({ element, history });
}

Checklist

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

elasticmachine · 2020-08-19T15:09:38Z

Pinging @elastic/kibana-core-ui (Team:Core UI)

elasticmachine · 2020-08-19T15:09:40Z

Pinging @elastic/kibana-platform (Team:Platform)

jportner

Security changes LGTM -- code review only

pgayvallet · 2020-08-19T17:08:52Z

ping @joshdover @myasonik @ryankeairns @lizozom PTAL regarding the actual API.

As there seems to be some misunderstanding, I prefer to clarify:

We won't be able to directly / automatically use the existing component (https://github.com/elastic/kibana/blob/master/src/plugins/navigation/public/top_nav_menu/top_nav_menu.tsx) without any changes inside the existing applications code.

What we are doing is exposing a way for applications to render the menu / topbar component inside the chrome header. But code adaptation will still be needed in every app to mount the component using the new setHeaderActionMenu API instead of manually rending inside their 'owned' dom as they were previously doing.

Now, regarding the 'raw' mountpoint vs duplicating the TopBarMenu / TopNavMenuData inside core:

We could, instead of the API proposed in this PR, expose a 'higher' level API that duplicates what is currently done in TopBarMenu.

That would mean:

duplicating the TopNavMenu code inside core

kibana/src/plugins/navigation/public/top_nav_menu/top_nav_menu_data.tsx

Lines 24 to 36 in bbee1f9

    
           export interface TopNavMenuData { 
        
             id?: string; 
        
             label: string; 
        
             run: TopNavMenuAction; 
        
             description?: string; 
        
             testId?: string; 
        
             className?: string; 
        
             disableButton?: boolean | (() => boolean); 
        
             tooltip?: string | (() => string | undefined); 
        
             emphasize?: boolean; 
        
             iconType?: string; 
        
             iconSide?: EuiButtonProps['iconSide']; 
        
           }

kibana/src/plugins/navigation/public/top_nav_menu/top_nav_menu.tsx

Lines 71 to 85 in 38b9a8d

    
           function renderMenu(className: string): ReactElement | null { 
        
             if (!config || config.length === 0) return null; 
        
             return ( 
        
               <EuiFlexGroup 
        
                 data-test-subj="top-nav" 
        
                 justifyContent="flexStart" 
        
                 alignItems="center" 
        
                 gutterSize="none" 
        
                 className={className} 
        
                 responsive={false} 
        
               > 
        
                 {renderItems()} 
        
               </EuiFlexGroup> 
        
             ); 
        
           }

and change setHeaderActionMenu to take the menu configuration instead of a MountPoint as parameter

export renderApp = ({ element, history, setHeaderActionMenu }: AppMountParameters) => {
   const { renderApp } = await import('./application');
   const { renderActionMenu } = await import('./action_menu');
   setHeaderActionMenuData([{id: 'foo', run: () => doSomething()...}]) // TopNavMenuData[]
   return renderApp({ element, history });
}

However, that would still require very similar changes for applications currently using the existing TopNavMenu, as they would still need to access and call the new API from the application's mount parameters, so I feel like the mount point is more permissive. We could eventually add an helper method to the navigation plugin:

export renderApp = ({ element, history, setHeaderActionMenu }: AppMountParameters) => {
   const { renderApp } = await import('./application');
   const { renderActionMenu } = await import('./action_menu');
   setHeaderActionMenu((element) => {
     return navigation.mountMenu(element, myMenuConfig); // Element, TopNavMenuData[]
   })
   return renderApp({ element, history });
}

But once again I have to insist on the fact that changes inside the applications currently using TopNavMenu will be required and can't be avoided.

Also note that if none of these changes would (probably) be too heavy, some might be non-trivial, and I think letting the core-ui and/or platform team performing these changes instead of the legitimate owners may be a risk.

For instance, the way this is done in the dashboard app is quite nested, inside a 'legacy' angular controller that atm does not have access to the app's AppMountParameters, meaning that this will need to be propagated down to it.

kibana/src/plugins/dashboard/public/application/dashboard_app_controller.tsx

Lines 681 to 684 in 9bef317

    
           const dashboardNavBar = document.getElementById('dashboardChrome'); 
        
           const updateNavBar = () => { 
        
             ReactDOM.render(<navigation.ui.TopNavMenu {...getNavBarProps()} />, dashboardNavBar); 
        
           };

Did not look at all usages, but others a probably similar.

joshdover · 2020-08-19T18:16:43Z

In order to smooth over the usage of this with React, I think we could offer a special MountPointPortal component that allows you to leverage a mount point within a React application without losing the ability for state updates to propagate into UI tree rendered inside the mount point. This could complement or even potentially replace the toMountPoint utility we have today.

Example:

const MountPointPortal: React.FC<{
  setMountPoint: (mountPoint: MountPoint<HTMLElement> | undefined) => void;
}> = ({ children, setMountPoint }) => {
  // state used to force re-renders when the element changes
  const [shouldRender, setShouldRender] = useState(false);
  const el = useRef<HTMLElement>();

  useEffect(() => {
    setMountPoint((element) => {
      el.current = element;
      setShouldRender(true);
      return () => {
        setShouldRender(false);
        el.current = undefined;
      };
    });

    return () => {
      setShouldRender(false);
      el.current = undefined;
    };
  }, [setMountPoint]);

  if (shouldRender && el.current) {
    return ReactDOM.createPortal(children, el.current);
  } else {
    return null;
  }
};

Applications could then use this pretty transparently:

const MyApp = ({ params }) => {
  const [someState, setSomeState] = useState(1);

  return (
    <Router history={params.history}>
      <MountPointPortal setMountPoint={params.setHeaderActionMenu}>
        <TopNavMenu someProp={someState} /> {/** state updates should still work here */}
      </MountPointPortal>
      {/** Rest of UI tree here */}
    </Router>
  );
}

I wasn't able to really test this on this PR since this is not yet integrated with the new Header PR.

ryankeairns · 2020-08-19T18:55:36Z

Re-capping for the non-eng audience, it sounds like there is an unavoidable bit of work that each app team (which currently uses the TopNavMenu component) will have to complete. Also, if I'm understanding, Josh's comment provides a way to lessen or at least simplify the implementation for each affected team. Are those statements generally accurate?

I'm asking 1) for my own understanding and 2) in order to start planning how we roll out this message/request.

Thanks for working on this important change!

pgayvallet · 2020-08-19T19:13:37Z

@ryankeairns both assertions are correct.

@joshdover I don't know why I didn't think about mounting the portal inside the mountpoint's provided element. I think this should work.

ping @elastic/kibana-app-arch to continue on that direction, I gonna need to make some changes and additions to the kibana_react and the navigation plugins:

Add the MountPointPortal utility FC to kibana_react
extract the menu rendering of TopNavMenu to it's own component (renderMenu)

I could use the component directly by passing showSearchBar=false, but I think that as in the header, the component will never have to render the search bar, it is a little cleaner to extract and reuse a new 'menu-only' component, wdyt?

add the utility mountMenu API, to achieve the wanted API format:

setHeaderActionMenu((element) => navigation.mountMenu(element, myMenuConfig))

Do you have any remark or objection regarding these changes?

cjcenizal

Dev Tools change LGTM.

…app-menu-api

peteharverson

ML code edit LGTM

…app-menu-api

lizozom · 2020-08-20T13:58:57Z

@pgayvallet If you plan to extract the menu rendering of TopNavMenu to it's own component, why can't TopNavMenu use the new API internally, so to avoid apps having to do extra work?

Not sure I understood the last extraction you described.

Also, does this API allow an app to change the menu during the runtime of the application (like switching a dashboard from and to edit mode). Just want to make sure this case was taken into account.

pgayvallet

Introduced the MountPointPortal component and adapted TopNavMenu to accept an optional setMenuMountPoint property.

@joshdover @lizozom PTAL

pgayvallet · 2020-08-24T09:14:04Z

src/plugins/navigation/public/top_nav_menu/_index.scss

+.kbnTopNavMenu {
+  padding: $euiSizeS 0;

-    .kbnTopNavItemEmphasized {
-      padding: 0 $euiSizeS;
-    }
+  .kbnTopNavItemEmphasized {
+    padding: 0 $euiSizeS;
  }
+}


The menu can now be rendered outside the TopNavMenu component due to the portal. I had to move the .kbnTopNavMenu styles outside of .kbnTopNavMenu__wrapper.

Note: there will probably be some style tweaking depending on the display, but this will have to be handled during the next step in #72331

The next EUI update will likely address some of this. I can assist Michail with any additional touch ups. Thanks!

pgayvallet · 2020-08-24T09:17:29Z

src/plugins/navigation/public/top_nav_menu/top_nav_menu.tsx

      <span className="kbnTopNavMenu__wrapper">
-        {renderMenu(className)}
+        {setMenuMountPoint ? (
+          <MountPointPortal setMountPoint={setMenuMountPoint}>
+            {renderMenu(className)}
+          </MountPointPortal>
+        ) : (
+          renderMenu(className)
+        )}
        {renderSearchBar()}
      </span>


I wonder if that would be preferable, when we are rendering via portal (setMenuMountPoint is defined), to duplicate the wrapper and do something like

if (setMenuMountPoint) { return ( <> <MountPointPortal setMountPoint={setMenuMountPoint}> {renderMenu(className)} </MountPointPortal> {renderSearchBar()} </> ); } else { return ( {renderMenu(className)} {renderSearchBar()} ); }

wdyt?

I prefer the duplicated code for readability 👍
May want to extract the className of the span into a variable that can be reused to prevent inconsistency bugs from changes to the classname.

pgayvallet · 2020-08-24T09:18:17Z

src/plugins/navigation/public/top_nav_menu/top_nav_menu.test.tsx

+      // menu is rendered outside of the component
+      expect(component.find(TOP_NAV_ITEM_SELECTOR).length).toBe(0);
+      expect(portalTarget.getElementsByTagName('BUTTON').length).toBe(menuItems.length);


As the portal is rendered outside the enzyme root element, we are forced to fallback to plain html API for this test.

joshdover · 2020-08-25T18:14:10Z

src/core/public/application/application_service.tsx

+  private refreshCurrentActionMenu = () => {
+    const appId = this.currentAppId$.getValue();
+    const currentActionMenu = appId ? this.appInternalStates.get(appId)?.actionMenu : undefined;
+    this.currentActionMenu$.next(currentActionMenu);


Maybe not worth doing until after we complete #74911, but it feels like there's a lot of state & update juggling going on in this service now. Seems like it would be better to have a single source of truth that everything else responds to. wdyt?

I agree, and I also think we should wait until legacy mode has been removed to cleanup that thing

joshdover · 2020-08-25T18:23:39Z

src/plugins/kibana_react/public/util/mount_point_portal.tsx

+ */
+export const MountPointPortal: React.FC<MountPointPortalProps> = ({ children, setMountPoint }) => {
+  // state used to force re-renders when the element changes
+  const [shouldRender, setShouldRender] = useState(false);


heh, I was hoping you had a more elegant idea than me, but glad it works!

AFAIK a state-based trick can't be avoid needed here unfortunately, as we can't 'observe' the changes on the reference.

joshdover · 2020-08-25T18:29:00Z

src/plugins/kibana_react/public/util/mount_point_portal.test.tsx

+
+    await refresh();
+
+    expect(portalTarget.textContent).toBe('');


nit: innerHTML may be a better property to test against since textContent can be an empty string even if there are other DOM node children

joshdover · 2020-08-25T18:29:47Z

src/plugins/kibana_react/public/util/mount_point_portal.tsx

+  }, [setMountPoint]);
+
+  if (shouldRender && el.current) {
+    return ReactDOM.createPortal(children, el.current);


Should we have a default error boundary here?

Wasn't sure. Added it.

joshdover · 2020-08-25T18:31:04Z

src/plugins/navigation/public/top_nav_menu/top_nav_menu.tsx

      <span className="kbnTopNavMenu__wrapper">
-        {renderMenu(className)}
+        {setMenuMountPoint ? (
+          <MountPointPortal setMountPoint={setMenuMountPoint}>
+            {renderMenu(className)}
+          </MountPointPortal>
+        ) : (
+          renderMenu(className)
+        )}
        {renderSearchBar()}
      </span>


I prefer the duplicated code for readability 👍
May want to extract the className of the span into a variable that can be reused to prevent inconsistency bugs from changes to the classname.

…app-menu-api

ryankeairns · 2020-08-31T19:27:16Z

@lizozom can we get your review and approval on this one?
It's a dependency for the new stacked header and is blocking some downstream work. Thanks!

…app-menu-api

kibanamachine · 2020-09-01T17:26:11Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 5759f93

Build metrics

@kbn/optimizer bundle module count

id	value	diff	baseline
kibanaReact	295	+1	294

async chunks size

id	value	diff	baseline
devTools	6.5KB	+69.0B	6.5KB

page load bundle size

id	value	diff	baseline
core	1.2MB	+2.0KB	1.2MB
kibanaReact	648.1KB	+6.0KB	642.1KB
ml	576.7KB	-101.0B	576.8KB
navigation	165.5KB	+848.0B	164.7KB
total		+8.7KB

History

💚 Build #71167 succeeded 339ed72
💚 Build #70676 succeeded 7ee6b10
💚 Build #70601 succeeded 950b2a2
💔 Build #70453 failed e34f402
💚 Build #69929 succeeded 70dad1d

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

* add `setHeaderActionMenu` to AppMountParameters * allow to remove the current menu by calling handler with undefined * update generated doc * updating snapshots * fix legacy tests * call renderApp with params * rename toMountPoint component file for consistency * add the MountPointPortal utility component * adapt TopNavMenu to add optional `setMenuMountPoint` prop * add kibanaReact as required bundle. * use innerHTML instead of textContent for portal tests * add error boundaries to portal component * improve renderLayout readability * duplicate wrapper in portal mode to avoid altering styles Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

ryankeairns · 2020-09-01T19:25:56Z

YESSSS thanks Pierre!

* add `setHeaderActionMenu` to AppMountParameters * allow to remove the current menu by calling handler with undefined * update generated doc * updating snapshots * fix legacy tests * call renderApp with params * rename toMountPoint component file for consistency * add the MountPointPortal utility component * adapt TopNavMenu to add optional `setMenuMountPoint` prop * add kibanaReact as required bundle. * use innerHTML instead of textContent for portal tests * add error boundaries to portal component * improve renderLayout readability * duplicate wrapper in portal mode to avoid altering styles Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

@ts-expect-error

* master: (223 commits) skip flaky suite (elastic#75724) [Reporting] Add functional test for Reports in non-default spaces (elastic#76053) [Enterprise Search] Fix various icons in dark mode (elastic#76430) skip flaky suite (elastic#76245) Add `auto` interval to histogram AggConfig (elastic#76001) [Resolver] generator uses setup_node_env (elastic#76422) [Ingest Manager] Support both zip & tar archives from Registry (elastic#76197) [Ingest Manager] Improve agent vs kibana version checks (elastic#76238) Manually building `KueryNode` for Fleet's routes (elastic#75693) remove dupe tinymath section (elastic#76093) Create APM issue template (elastic#76362) Delete unused file. (elastic#76386) [SECURITY_SOLUTION][ENDPOINT] Trusted Apps Create API (elastic#76178) [Detections Engine] Add Alert actions to the Timeline (elastic#73228) [Dashboard First] Library Notification (elastic#76122) [Maps] Add mvt support for ES doc sources (elastic#75698) Add setHeaderActionMenu API to AppMountParameters (elastic#75422) [ML] Remove "Are you sure" from data frame analytics jobs (elastic#76214) [yarn] remove typings-tester, use @ts-expect-error (elastic#76341) [Reporting/CSV] Do not fail the job if scroll ID can not be cleared (elastic#76014) ...

pgayvallet added 3 commits August 19, 2020 14:11

add setHeaderActionMenu to AppMountParameters

96df7a9

allow to remove the current menu by calling handler with undefined

4556772

update generated doc

96d3494

pgayvallet added v7.10.0 release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc labels Aug 19, 2020

pgayvallet mentioned this pull request Aug 19, 2020

[Global header] Create API to register application menu links #68524

Closed

pgayvallet added the REASSIGN from Team:Core UI Deprecated label for old Core UI team label Aug 19, 2020

pgayvallet added 2 commits August 19, 2020 14:59

updating snapshots

eb70d9c

fix legacy tests

e595519

pgayvallet marked this pull request as ready for review August 19, 2020 15:09

pgayvallet requested review from a team as code owners August 19, 2020 15:09

jportner approved these changes Aug 19, 2020

View reviewed changes

cjcenizal approved these changes Aug 19, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into kbn-68524-header-…

403a5f1

…app-menu-api

peteharverson approved these changes Aug 20, 2020

View reviewed changes

pgayvallet added 3 commits August 20, 2020 11:18

Merge remote-tracking branch 'upstream/master' into kbn-68524-header-…

22fef27

…app-menu-api

call renderApp with params

d3a2c8e

Merge remote-tracking branch 'upstream/master' into kbn-68524-header-…

ea91712

…app-menu-api

pgayvallet added 2 commits August 24, 2020 09:20

rename toMountPoint component file for consistency

ab75d27

add the MountPointPortal utility component

20685b3

pgayvallet requested a review from a team as a code owner August 24, 2020 08:31

adapt TopNavMenu to add optional setMenuMountPoint prop

6405ab5

pgayvallet requested a review from a team as a code owner August 24, 2020 09:08

pgayvallet commented Aug 24, 2020

View reviewed changes

add kibanaReact as required bundle.

70dad1d

joshdover reviewed Aug 25, 2020

View reviewed changes

pgayvallet added 4 commits August 26, 2020 09:55

Merge remote-tracking branch 'upstream/master' into kbn-68524-header-…

bdbaa6a

…app-menu-api

use innerHTML instead of textContent for portal tests

0d02a2c

add error boundaries to portal component

530e031

improve renderLayout readability

e34f402

joshdover approved these changes Aug 26, 2020

View reviewed changes

pgayvallet added 3 commits August 26, 2020 19:37

Merge remote-tracking branch 'upstream/master' into kbn-68524-header-…

950b2a2

…app-menu-api

Merge remote-tracking branch 'upstream/master' into kbn-68524-header-…

89e3363

…app-menu-api

duplicate wrapper in portal mode to avoid altering styles

7ee6b10

pgayvallet removed the request for review from a team August 26, 2020 19:49

Merge remote-tracking branch 'upstream/master' into kbn-68524-header-…

339ed72

…app-menu-api

lizozom approved these changes Sep 1, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into kbn-68524-header-…

5759f93

…app-menu-api

pgayvallet merged commit 981691d into elastic:master Sep 1, 2020

pgayvallet mentioned this pull request Sep 1, 2020

[7.x] Add setHeaderActionMenu API to AppMountParameters (#75422) #76408

Merged

myasonik mentioned this pull request Sep 1, 2020

Stacked headers and navigational search #72331

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add setHeaderActionMenu API to AppMountParameters #75422

Add setHeaderActionMenu API to AppMountParameters #75422

pgayvallet commented Aug 19, 2020 •

edited

Loading

elasticmachine commented Aug 19, 2020

elasticmachine commented Aug 19, 2020

jportner left a comment

pgayvallet commented Aug 19, 2020

joshdover commented Aug 19, 2020

ryankeairns commented Aug 19, 2020

pgayvallet commented Aug 19, 2020 •

edited

Loading

cjcenizal left a comment

peteharverson left a comment

lizozom commented Aug 20, 2020

pgayvallet left a comment

pgayvallet Aug 24, 2020

ryankeairns Aug 24, 2020

pgayvallet Aug 24, 2020

joshdover Aug 25, 2020

pgayvallet Aug 24, 2020

joshdover Aug 25, 2020

pgayvallet Aug 25, 2020

joshdover Aug 25, 2020

pgayvallet Aug 25, 2020

joshdover Aug 25, 2020

joshdover Aug 25, 2020

pgayvallet Aug 26, 2020

joshdover Aug 25, 2020

ryankeairns commented Aug 31, 2020

kibanamachine commented Sep 1, 2020

ryankeairns commented Sep 1, 2020

Add setHeaderActionMenu API to AppMountParameters #75422

Add setHeaderActionMenu API to AppMountParameters #75422

Conversation

pgayvallet commented Aug 19, 2020 • edited Loading

Summary

Example

Checklist

elasticmachine commented Aug 19, 2020

elasticmachine commented Aug 19, 2020

jportner left a comment

Choose a reason for hiding this comment

pgayvallet commented Aug 19, 2020

joshdover commented Aug 19, 2020

ryankeairns commented Aug 19, 2020

pgayvallet commented Aug 19, 2020 • edited Loading

cjcenizal left a comment

Choose a reason for hiding this comment

peteharverson left a comment

Choose a reason for hiding this comment

lizozom commented Aug 20, 2020

pgayvallet 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

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

ryankeairns commented Aug 31, 2020

kibanamachine commented Sep 1, 2020

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

async chunks size

page load bundle size

History

ryankeairns commented Sep 1, 2020

pgayvallet commented Aug 19, 2020 •

edited

Loading

pgayvallet commented Aug 19, 2020 •

edited

Loading