Implement new security solution wrapper #100405

michaelolo24 · 2021-05-20T14:34:30Z

Summary

This PR introduces the new KibanaPageTemplate nav wrapper to the security solution as detailed here #98359. Of note:

(sample image below)

The template_wrapper folder was added in the public/app/home folder. It contains the new KibanaPageTemplate as well as the code for the global kql header and bottom bar (containing the timeline)
Added a navigation hook within the components/navigation folder for the side nav to meet the the reqs for the solutionNav prop. Pulled in functionality from the TabNavigation component to keep state in sync as well as telemetry. Also, updated the urlState makeMapStateToProps a hook and updated the name for the primary navigation.
The Add Data and Ml Job Settings were moved to the Kibana fixed header to get some more vertical space back. The code for this can be found in public/app/global_header
WrapperPage was renamed to SecuritySolutionPageWrapper. Initially made this change as the pageTemplate code lived in this file, but decided to keep it when I migrated it out into the home folder. Can change it back if people don't prefer this naming scheme, but it seemed in line with other solutions. Note: We have a few wrappers that may eventually be able to be consolidated once all the nav changes are completed.
Moved drag related classnames into their own file as they were causing a circular dependency in drag helpers
Updated the EuiPanel page level components within Security Solution to use the hasBorder prop so they no longer have a shadow border.

Still To Be Done

Empty pages need to be updated to match the new design. That will be done when we have new mocks
We do not yet have the ability to collapse the side nav. This has been discussed internally and the decision as of now is to move forward without that functionality with the hope that it comes in before 7.14.
Another kbnAppWrapper as of now, will need to be added at the kibana app level to force full height for all of the pages
Pages will be broken into new sections once [Security Solution] Application register deepLinks instead of meta.searchDeepLinks #100129 is merged

New Solution Nav

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
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

x-pack/plugins/security_solution/public/app/home/index.tsx

michaelolo24 · 2021-06-09T20:27:37Z

x-pack/plugins/security_solution/public/common/components/drag_and_drop/helpers.ts

@@ -247,12 +247,6 @@ export const addFieldToTimelineColumns = ({
 */
 export const DRAG_TYPE_FIELD = 'drag-type-field';

-/** This class is added to the document body while dragging */


These were causing a cyclic dependency so moved them into their own classname constants file

michaelolo24 · 2021-06-09T20:29:48Z

x-pack/plugins/security_solution/public/common/components/navigation/index.tsx


-  useEffect(() => {
-    if (pathName || pageName) {


primary changes in this file were renaming the component and updating it to purely use hooks. See useUrlState. Also the primary nav no longer uses this but uses the hook in this file use_security_solution_navigation

michaelolo24 · 2021-06-09T20:32:30Z

x-pack/plugins/security_solution/public/common/components/page/index.tsx

@@ -27,25 +27,6 @@ SecuritySolutionAppWrapper.displayName = 'SecuritySolutionAppWrapper';
  and `EuiPopover`, `EuiToolTip` global styles
 */
 export const AppGlobalStyle = createGlobalStyle<{ theme: { eui: { euiColorPrimary: string } } }>`
-  // fixes double scrollbar on views with EventsTable


These overrides were causing visual issues with the new template.

michaelolo24 · 2021-06-09T20:35:23Z

x-pack/plugins/security_solution/public/timelines/components/flyout/bottom_bar/index.tsx

-const SHOW_HIDE_GLOBAL_TRANSLATE_Y = 50; // px
-const SHOW_HIDE_TIMELINE_TRANSLATE_Y = 0; // px
-
-const Container = styled.div.attrs<{ $isGlobal: boolean }>(({ $isGlobal = true }) => ({


The template bottomBar handles a lot of this logic already, but any additional timeline specific logic was added to the styles seen in template_wrapper/index

elasticmachine · 2021-06-09T20:39:33Z

Pinging @elastic/security-solution (Team: SecuritySolution)

elasticmachine · 2021-06-09T20:39:35Z

Pinging @elastic/security-threat-hunting (Team:Threat Hunting)

XavierM · 2021-06-21T13:54:49Z

x-pack/plugins/security_solution/public/common/components/drag_and_drop/drag_classnames.ts

+ * 2.0.
+ */
+
+/** This class is added to the document body while dragging */


oh this is going to be fun!!! I moved them in packages to be shared between security solution and timelines

Yea... 😅

x-pack/plugins/security_solution/public/common/components/navigation/index.tsx

...rity_solution/public/common/components/navigation/use_security_solution_navigation/index.tsx

XavierM · 2021-06-21T13:58:35Z

...lic/common/components/navigation/use_security_solution_navigation/use_primary_navigation.tsx

+  timeline,
+  timerange,
+}: PrimaryNavigationProps): KibanaPageTemplateProps['solutionNav'] => {
+  const mapLocationToTab = useCallback(


you do not need to use useCallback here, it does not matter if the mapLocationToTab reference change.

All of this is taken from the tab navigation file to keep in sync with the logic there, but it actually makes sense to have it based on the function being called in the useEffect

jonathan-buttner

Thanks for all the comments Mike! 💪

jonathan-buttner · 2021-06-21T15:35:24Z

x-pack/plugins/security_solution/public/app/home/global_header/index.tsx

+              <EuiHeaderLink
+                color="primary"
+                data-test-subj="add-data"
+                href={`${basePath}${ADD_DATA_PATH}`}


nit: I think we could do href={http.basePath.prepend(ADD_DATA_PATH)} that'll prepend base path to the passed in URL.

Yea, that worked. Updated it thanks!

jonathan-buttner · 2021-06-21T20:01:29Z

...ublic/common/components/navigation/use_security_solution_navigation/use_navigation_items.tsx

+    // should be consistent across every render. The primary nav tabs here are from a constants file
+    // not dynamically generated, so their order will remain consistent across renders.
+    // eslint-disable-next-line react-hooks/rules-of-hooks
+    const { formatUrl } = useFormatUrl(((pageId ?? id) as unknown) as SecurityPageName);


I'm not super familiar with this code but could we set pageId on all the tabs here: https://github.com/elastic/kibana/blob/master/x-pack/plugins/security_solution/public/app/home/home_navigations.tsx#L21 ? Then we could avoid the casting right?

It might also help clean up a few places where we need to check whether id or pageId is set.

Was actually able to remove this entirely. Kept a lot of logic from the tab navigation that wasn't necessary

jonathan-buttner · 2021-06-21T20:02:13Z

...ublic/common/components/navigation/use_security_solution_navigation/use_navigation_items.tsx

+    const handleClick = (ev: React.MouseEvent) => {
+      ev.preventDefault();
+      if (id in SecurityPageName && pageId == null) {
+        navigateToApp(`${APP_ID}:${id}`, { path: urlSearch });


I think navigateToApp does a history.push or history.replace under the scenes so just curious why we couldn't use navigateToApp for both situations here?

Was just maintaining the logic that was used in the tabNavigation, but this isn't necessary for the primary navigation so I've removed it.

I think the reason was to avoid a mount of the app

but all this stuff will be changed in follow up PR with @semd

jonathan-buttner · 2021-06-21T20:18:36Z

...ublic/common/components/navigation/use_security_solution_navigation/use_navigation_items.tsx

+    // Rules of hooks prevents hooks from being called in callbacks as the order of the hooks
+    // should be consistent across every render. The primary nav tabs here are from a constants file
+    // not dynamically generated, so their order will remain consistent across renders.
+    // eslint-disable-next-line react-hooks/rules-of-hooks


It's probably not worth the effort but would the alternative be to return a callback instead of navItems that could be called while iterating over navTabs? So the caller of usePrimaryNavigationItems would have to do the looping and call the callback returned by usePrimaryNavigationItems. Or would that not work?

Thanks for the great comment by the way though!

Yea, that would work. Actually probably a better route to go to not have to disable any rules of hooks that could cause bugginess in the future if anything changed. Turns out though that I didn't need this code at all as the primary nav doesn't have sub-tabs. Once @semd's changes are in we should be able to get rid of all pageId references and do a final nav cleanup

jonathan-buttner · 2021-06-21T20:25:44Z

...urity_solution/public/detections/components/callouts/need_admin_for_update_callout/index.tsx

+  return shouldShowCallout ? (
+    <>
+      <CallOutPersistentSwitcher
+        condition={shouldShowCallout}


Would this ever receive false? Also I think the only place CallOutPersistentSwitcher is used is here. I think we could remove the condition prop right?

Nope, fixed it. Thanks!

XavierM

let me know if you need help to merge with master and then I will approve it

michaelolo24 · 2021-06-23T12:16:44Z

let me know if you need help to merge with master and then I will approve it

@XavierM No worries, I merged master earlier this morning, updated the references, and everything looks good. 🤞🏾

kibanamachine · 2021-06-23T13:45:59Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`securitySolution`	2189	2194	+5

Async chunks

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

id	before	after	diff
`cases`	507.5KB	507.8KB	+292.0B
`securitySolution`	6.5MB	6.5MB	-365.0B
total			-73.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`	411.5KB	412.4KB	+952.0B
`securitySolution`	197.8KB	197.7KB	-114.0B
total			+838.0B

Unknown metric groups

References to deprecated APIs

id	before	after	diff
`securitySolution`	390	394	+4

History

💔 Build #133370 failed 42d14d4
💔 Build #133342 failed 928eb45
💚 Build #133234 succeeded 0b9a590
💔 Build #133201 failed d1595f0
💚 Build #133151 succeeded d3a9e26

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

michaelolo24 · 2021-06-23T14:01:00Z

x-pack/plugins/security_solution/public/common/components/header_global/index.tsx

-
-      // build a list of tabs to exclude
-      const tabsToExclude = new Set<string>([
-        ...(hideDetectionEngine ? [SecurityPageName.detections] : []),


@XavierM did not see this used or called anywhere?

good, it was an old Feature flag fro 7.5

XavierM

LGTM, I reviewed code and tested locally.

Co-authored-by: cchaos <caroline.horn@elastic.co> # Conflicts: # x-pack/plugins/security_solution/cypress/integration/timelines/data_providers.spec.ts # x-pack/plugins/security_solution/cypress/integration/timelines/pagination.spec.ts

* Implement new security solution wrapper (#100405) Co-authored-by: cchaos <caroline.horn@elastic.co> # Conflicts: # x-pack/plugins/security_solution/cypress/integration/timelines/data_providers.spec.ts # x-pack/plugins/security_solution/cypress/integration/timelines/pagination.spec.ts * test fixes

michaelolo24 force-pushed the security-solution-nav-update branch 4 times, most recently from 0651ca2 to 71a6366 Compare May 28, 2021 19:59

michaelolo24 force-pushed the security-solution-nav-update branch 3 times, most recently from 29e464e to 360e8aa Compare June 4, 2021 13:26

michaelolo24 added v7.14.0 v8.0.0 labels Jun 4, 2021

michaelolo24 force-pushed the security-solution-nav-update branch 3 times, most recently from 97ab77a to 871d95f Compare June 9, 2021 18:51

michaelolo24 added Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Threat Hunting Security Solution Threat Hunting Team release_note:skip Skip the PR/issue when compiling release notes labels Jun 9, 2021

michaelolo24 commented Jun 9, 2021

View reviewed changes

x-pack/plugins/security_solution/public/app/home/index.tsx Outdated Show resolved Hide resolved

michaelolo24 commented Jun 9, 2021

View reviewed changes

michaelolo24 marked this pull request as ready for review June 9, 2021 20:39

michaelolo24 requested review from a team as code owners June 9, 2021 20:39

michaelolo24 requested a review from cchaos June 9, 2021 22:33

michaelolo24 added 4 commits June 15, 2021 06:41

Implement new security solution wrapper

6bde37b

nav changes

7234d3b

added ml jobs and add data to topbar

ff22379

trying to fix/finalize styles

4ddcdec

XavierM reviewed Jun 21, 2021

View reviewed changes

x-pack/plugins/security_solution/public/common/components/navigation/index.tsx Outdated Show resolved Hide resolved

XavierM reviewed Jun 21, 2021

View reviewed changes

...rity_solution/public/common/components/navigation/use_security_solution_navigation/index.tsx Outdated Show resolved Hide resolved

XavierM reviewed Jun 21, 2021

View reviewed changes

michaelolo24 added 2 commits June 21, 2021 12:27

Merge branch 'master' into security-solution-nav-update

6a33d8b

undo url state changes

7a4029c

michaelolo24 force-pushed the security-solution-nav-update branch from beccdde to 7a4029c Compare June 21, 2021 18:55

jonathan-buttner reviewed Jun 21, 2021

View reviewed changes

michaelolo24 added 5 commits June 22, 2021 09:58

pr changes

d3a9e26

addition pr changes

d1595f0

remove unused imports

0b9a590

Merge branch 'master' into security-solution-nav-update

928eb45

update cases visibility tests

42d14d4

XavierM reviewed Jun 23, 2021

View reviewed changes

michaelolo24 added 4 commits June 23, 2021 07:08

fix cypress test

d1d992a

remove only

3815b21

Merge branch 'master' into security-solution-nav-update

4b0847c

update to use t-grid

f4465c0

michaelolo24 commented Jun 23, 2021

View reviewed changes

XavierM approved these changes Jun 23, 2021

View reviewed changes

michaelolo24 merged commit 702661d into elastic:master Jun 23, 2021

michaelolo24 deleted the security-solution-nav-update branch June 23, 2021 15:00

michaelolo24 restored the security-solution-nav-update branch June 23, 2021 17:50

michaelolo24 mentioned this pull request Jun 24, 2021

Implement new security solution wrapper (#100405) #103243

Closed

michaelolo24 mentioned this pull request Jun 24, 2021

[7.x] Implement new security solution wrapper (#100405) #103244

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement new security solution wrapper #100405

Implement new security solution wrapper #100405

michaelolo24 commented May 20, 2021 •

edited

Loading

michaelolo24 Jun 9, 2021

michaelolo24 Jun 9, 2021

michaelolo24 Jun 9, 2021

michaelolo24 Jun 9, 2021

elasticmachine commented Jun 9, 2021

elasticmachine commented Jun 9, 2021

XavierM Jun 21, 2021

michaelolo24 Jun 22, 2021

XavierM Jun 21, 2021

michaelolo24 Jun 21, 2021

jonathan-buttner left a comment

jonathan-buttner Jun 21, 2021

michaelolo24 Jun 22, 2021

jonathan-buttner Jun 21, 2021

michaelolo24 Jun 22, 2021

jonathan-buttner Jun 21, 2021

michaelolo24 Jun 22, 2021

XavierM Jun 23, 2021

XavierM Jun 23, 2021

jonathan-buttner Jun 21, 2021

jonathan-buttner Jun 21, 2021

michaelolo24 Jun 22, 2021

jonathan-buttner Jun 21, 2021

michaelolo24 Jun 22, 2021

XavierM left a comment •

edited

Loading

michaelolo24 commented Jun 23, 2021

kibanamachine commented Jun 23, 2021

References to deprecated APIs

michaelolo24 Jun 23, 2021

XavierM Jun 23, 2021

XavierM left a comment

Implement new security solution wrapper #100405

Implement new security solution wrapper #100405

Conversation

michaelolo24 commented May 20, 2021 • edited Loading

Summary

New Solution Nav

Checklist

For maintainers

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

elasticmachine commented Jun 9, 2021

elasticmachine commented Jun 9, 2021

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

jonathan-buttner 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

XavierM left a comment • edited Loading

Choose a reason for hiding this comment

michaelolo24 commented Jun 23, 2021

kibanamachine commented Jun 23, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

References to deprecated APIs

History

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XavierM left a comment

Choose a reason for hiding this comment

michaelolo24 commented May 20, 2021 •

edited

Loading

XavierM left a comment •

edited

Loading