[Endpoint] Task/add nav bar #58604

kevinlog · 2020-02-26T15:33:12Z

Summary

A PR to add a header nav to the endpoint app. Uses EuiTabs with useHistory to add the urls to our router history. Thanks to RouteCapture, will properly update the URL in the redux store.

Now contains tests.

Implements https://github.com/elastic/endpoint-app-team/issues/174

Redux:

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility, including a check against IE11

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2020-02-26T15:47:36Z

Pinging @elastic/endpoint-app-team (Feature:Endpoint)

paul-tavares

@kevinlog should the header navigation component be split off into its own separate component instead of placing it into the index.tsx?
Also, the array of Nav tabs might become useful as well in other places (later) if we want to reference a centralized place that holds the to path to the different application pages (in order to build links, or redirect users).

paul-tavares · 2020-02-26T20:36:12Z

x-pack/plugins/endpoint/public/applications/endpoint/index.tsx

+function renderNavTabs(tabs: NavTabs[]) {
+  return tabs.map((tab, index) => {
+    return (
+      <NavLink


We should probably use theEuiTabs component for this and not the NavLink from react-router. EuiTab allows for href to be set as well as a onClick callback - the callback would be where you can handle calling react-router's history.push. We might need to styled the EuiTabs a little (see example in Ingest just committed recently)

Also - one of the thing the EUI team discourages is to use the internal class names on components directly like this :(

paul-tavares · 2020-02-26T20:41:10Z

x-pack/plugins/endpoint/public/applications/endpoint/index.tsx

+const navTabs: NavTabs[] = [
+  {
+    name: 'home',
+    display: 'Home',


Need to i18n all display values

paul-tavares · 2020-02-26T23:02:48Z

@kevinlog just read your question in the PR description re: redux ++ middleware.
You might not even need it. The determination on whether a Tab is selected or not can be done using useLocation().pathname and comparing that to the value of each Nav's .to value.

kevinlog · 2020-02-27T04:10:30Z

@paul-tavares thanks for the detailed comments. I adjusted the PR to use EuiTabs directly, let me know what you think

paul-tavares

LGTM

Only minor change. Thanks

x-pack/plugins/endpoint/public/applications/endpoint/components/header_nav.tsx

paul-tavares · 2020-02-27T15:51:19Z

x-pack/plugins/endpoint/public/applications/endpoint/components/header_nav.tsx

+        <EuiTab
+          data-testid={`${tab.id}Link`}
+          key={index}
+          onClick={() => {


I'm ok this now having an href at this time, but we should decide if we should include one in the tab in order to enable the user to open in separate window/tab.

paul-tavares · 2020-02-27T15:56:26Z

Actually: question - do you need test cases for this? Probably a few, at least to validate that we're correctly setting the selected tab. Also, what happens when we get routed to an invalid URI (I assume no tab is selected, but nav is still visible).

…s/header_nav.tsx Co-Authored-By: Paul Tavares <56442535+paul-tavares@users.noreply.github.com>

kevinlog · 2020-02-28T15:17:10Z

@elasticmachine merge upstream

elasticmachine · 2020-02-28T15:18:33Z

Pinging @elastic/endpoint-response (Team:Endpoint Response)

elasticmachine · 2020-02-28T15:18:35Z

Pinging @elastic/endpoint-management (Team:Endpoint Management)

elasticmachine · 2020-02-28T15:18:36Z

Pinging @elastic/endpoint-data-visibility-team (Team:Endpoint Data Visibility)

…sk/add-nav-bar

paul-tavares · 2020-02-28T16:03:51Z

x-pack/plugins/endpoint/public/applications/endpoint/components/header_nav.tsx

+          data-testid={`${tab.id}EndpointTab`}
+          data-test-subj={`${tab.id}EndpointTab`}
+          key={index}
+          href={`/app/endpoint${tab.href}`}


🔴 You can't hardcode the /app/endpoint or else you break when kibana is running with base-path. I think @kqualters-elastic has added the use of kibana-react to our application, wich means we can use their hook to retrieve coreStart and then get the base path from the http service (see this: #58288 (comment)).

An alternative for now is to add a basePath prop to this component and then send it in from index.tsx since you should have that info. available there.

paul-tavares · 2020-02-28T16:05:02Z

x-pack/plugins/endpoint/public/applications/endpoint/index.tsx

@@ -41,6 +42,7 @@ const AppRoot: React.FunctionComponent<RouterProps> = React.memo(({ basename, st
    <I18nProvider>
      <BrowserRouter basename={basename}>
        <RouteCapture>
+          <HeaderNavigation />


Here, you can pass in basename to HeaderNavigation

oatkiller · 2020-02-28T16:11:17Z

x-pack/plugins/endpoint/public/applications/endpoint/components/header_nav.tsx

+          data-test-subj={`${tab.id}EndpointTab`}
+          key={index}
+          href={`/app/endpoint${tab.href}`}
+          onClick={(event: MouseEvent) => {


An idea for how we could emulate react-router when using Eui components.
oatkiller@d6861d8

Things this handles:

useCallback for the callback

doesn't navigate when modifier keys are pressed

can call an additional onClick handler if needed (navigate via a link AND have a callback)

doesn't navigate if event is 'defaultPrevented'

logic copied from react-router's link, so behavior is consistent

should work in EuiLink as well

see react router's implementation for ideas
https://github.com/ReactTraining/react-router/blob/master/packages/react-router-dom/modules/Link.js

Great approach that prevents us having to wrap Eui components 👍

@oatkiller @paul-tavares this doesn't quite work yet, I broke this out to another ticket: https://github.com/elastic/endpoint-app-team/issues/230

Are you OK to merge this as is and I'll investigate the next step for this?

bkimmel · 2020-02-28T16:22:42Z

x-pack/plugins/endpoint/public/applications/endpoint/components/header_nav.tsx

+    name: i18n.translate('xpack.endpoint.headerNav.management', {
+      defaultMessage: 'Management',
+    }),
+    href: '/management',


@paul-tavares does the basePath concern apply here as well?

@bkimmel no - here its ok because we're using this value against react-router which we initialize with a basePath (the one provided during plugin initialization). So these should be (absolute) relative to basePath

bkimmel · 2020-02-28T16:35:54Z

x-pack/plugins/endpoint/public/applications/endpoint/components/header_nav.tsx

+  },
+];
+
+const Tabs = styled(EuiTabs)`


❔ There is also EuiTabbedContent which seems like it would capture some more of what you're trying to accomplish here. Can you elaborate on why you didn't use the EuiTabbedContent approach?

@bkimmel that styled doesn't make a difference that I can see, so I removed it. m See this for the original context #58604 (comment)

paul-tavares

LGTM 👍

kibanamachine · 2020-02-28T19:24:51Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: d9de5a7

History

💔 Build #29751 failed d8425ec
💔 Build #29735 failed 9ae3933
💔 Build #29725 failed e90a9ac
💚 Build #29357 succeeded 7d2d58a
💚 Build #29155 succeeded b9b37d9

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

kibanamachine · 2020-02-29T19:28:44Z

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

kibanamachine · 2020-03-01T19:38:43Z

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

…dex-server-side * 'master' of github.com:elastic/kibana: (34 commits) [Upgrade Assistant] Remove "boom" from reindex service (elastic#58715) [data] Clean up QueryStringInput unit tests (elastic#58704) [SIEM] Detection Fix typo in Adobe Hijack Persistence rule (elastic#58804) Restores [SIEM][CASE] Init Configure Case Page (elastic#58121) (elastic#58924) Skips additional failing Ingest Manager integration tests Skips failing Ingest Manager integration tests Move dev tools styles to NP (elastic#58855) change to have kibana --ssl cli option use more recent certs (elastic#57933) disable failing suite (elastic#58942) Don't start pollEsNodesVersion unless someone subscribes (elastic#56923) Do not write UUID file during optimize process (elastic#58899) [Endpoint] Task/add nav bar (elastic#58604) [Metric Alerts] Add backend support for multiple expressions per alert (elastic#58672) [Metrics Alerts] Fix alerting on a rate aggregation (elastic#58789) disable flaky suite (elastic#55953) Revert "[SIEM] apollo@3 (elastic#51926)" and "[SIEM][CASE] Init Confi… (elastic#58806) [resubmit] Prep agg types for new platform (elastic#58893) [Lens] Allow number formatting within Lens (elastic#56253) [Autocomplete] Use settings from config rather than UI settings (elastic#58784) Improve action and trigger types (elastic#58657) ... # Conflicts: # x-pack/plugins/upgrade_assistant/server/routes/reindex_indices/reindex_indices.ts

kibanamachine · 2020-03-02T19:48:44Z

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

* Add tabs to the Endpoint app. Uses EuiTabs and browser history for integration with react-router Co-authored-by: Paul Tavares <56442535+paul-tavares@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Paul Tavares <56442535+paul-tavares@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

kevinlog added 2 commits February 26, 2020 06:22

add nav bar

db1e116

style, refactor

b9b37d9

oatkiller added the Feature:Endpoint Elastic Endpoint feature label Feb 26, 2020

kevinlog requested review from oatkiller, paul-tavares and parkiino February 26, 2020 15:49

paul-tavares reviewed Feb 26, 2020

View reviewed changes

kevinlog added 4 commits February 26, 2020 19:59

use EuiTabs, use onClick to push to history

11db5b7

revert change

69c0ecb

remove unused import

625635c

i18n for nav names

7d2d58a

paul-tavares approved these changes Feb 27, 2020

View reviewed changes

kevinlog and others added 3 commits February 28, 2020 09:57

add tests

09382af

Update x-pack/plugins/endpoint/public/applications/endpoint/component…

e90a9ac

…s/header_nav.tsx Co-Authored-By: Paul Tavares <56442535+paul-tavares@users.noreply.github.com>

correct build, add href

3e928f3

kevinlog marked this pull request as ready for review February 28, 2020 15:16

Merge branch 'master' into task/add-nav-bar

9ae3933

kevinlog added release_note:skip Skip the PR/issue when compiling release notes v7.7.0 Team:Endpoint Data Visibility Team managing the endpoint resolver Team:Endpoint Management Team:Endpoint Response Endpoint Response Team labels Feb 28, 2020

kevinlog changed the title ~~Task/add nav bar~~ [Endpoint] Task/add nav bar Feb 28, 2020

james-elastic approved these changes Feb 28, 2020

View reviewed changes

kevinlog added 2 commits February 28, 2020 11:02

fix type and typo

59e822d

Merge branch 'task/add-nav-bar' of github.com:kevinlog/kibana into ta…

d8425ec

…sk/add-nav-bar

paul-tavares reviewed Feb 28, 2020

View reviewed changes

oatkiller reviewed Feb 28, 2020

View reviewed changes

bkimmel reviewed Feb 28, 2020

View reviewed changes

kevinlog added 2 commits February 28, 2020 12:24

add basepath

94d5f0d

adjust format

d9de5a7

paul-tavares approved these changes Feb 28, 2020

View reviewed changes

oatkiller approved these changes Feb 28, 2020

View reviewed changes

kevinlog merged commit b46a335 into elastic:master Feb 28, 2020

kevinlog deleted the task/add-nav-bar branch February 28, 2020 19:25

kevinlog mentioned this pull request Feb 28, 2020

[7.x] [Endpoint] Task/add nav bar (#58604) #58934

Merged

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Feb 29, 2020

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Endpoint] Task/add nav bar #58604

[Endpoint] Task/add nav bar #58604

kevinlog commented Feb 26, 2020 •

edited

Loading

elasticmachine commented Feb 26, 2020

paul-tavares left a comment

paul-tavares Feb 26, 2020

paul-tavares Feb 26, 2020

paul-tavares commented Feb 26, 2020

kevinlog commented Feb 27, 2020

paul-tavares left a comment

paul-tavares Feb 27, 2020

paul-tavares commented Feb 27, 2020

kevinlog commented Feb 28, 2020

elasticmachine commented Feb 28, 2020

elasticmachine commented Feb 28, 2020

elasticmachine commented Feb 28, 2020

paul-tavares Feb 28, 2020

paul-tavares Feb 28, 2020

oatkiller Feb 28, 2020

paul-tavares Feb 28, 2020

kevinlog Feb 28, 2020

bkimmel Feb 28, 2020

paul-tavares Feb 28, 2020

bkimmel Feb 28, 2020

kevinlog Feb 28, 2020

paul-tavares left a comment

kibanamachine commented Feb 28, 2020

kibanamachine commented Feb 29, 2020

kibanamachine commented Mar 1, 2020

kibanamachine commented Mar 2, 2020

[Endpoint] Task/add nav bar #58604

[Endpoint] Task/add nav bar #58604

Conversation

kevinlog commented Feb 26, 2020 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Feb 26, 2020

paul-tavares left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paul-tavares commented Feb 26, 2020

kevinlog commented Feb 27, 2020

paul-tavares left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paul-tavares commented Feb 27, 2020

kevinlog commented Feb 28, 2020

elasticmachine commented Feb 28, 2020

elasticmachine commented Feb 28, 2020

elasticmachine commented Feb 28, 2020

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

paul-tavares left a comment

Choose a reason for hiding this comment

kibanamachine commented Feb 28, 2020

💚 Build Succeeded

History

kibanamachine commented Feb 29, 2020

kibanamachine commented Mar 1, 2020

kibanamachine commented Mar 2, 2020

kevinlog commented Feb 26, 2020 •

edited

Loading