Add side navigation component for serverless search by tsullivan · Pull Request #156465 · elastic/kibana

tsullivan · 2023-05-03T05:29:09Z

Summary

This PR implements a side navigation tree for Serverless Enterprise Search in a way that uses the earliest deliverables from the AppEx Shared UX team.

Technical doc for Side Nav: https://docs.google.com/document/d/1ew8KYl6ROs_V7jeIXgeP_C9YgkYK2IPtuceo6KVF_jE/edit#

Screenshots

Before

After

Known issues

Page scroll is not working when Chrome renders in serverless project mode. This issue exists prior to this PR
Highlighting the active link in the nav, and rendering the correct breadcrumb trail, is planned for a future PR

Checklist

Delete any items that are not applicable to this PR.

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)
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

tsullivan · 2023-05-03T05:33:55Z

x-pack/plugins/serverless_search/public/layout/nav.tsx

+    name: '',
+    id: 'root',
+    items: [
+      { id: 'overview', name: 'Overview', href: '/app/enterprise_search/overview' },


NOTE: These links were copied from the serverless Search nav from an earlier POC: 89bd09c#diff-a3106909413d188596dc1162e4fc5d4f884f4fd9b3a4eca6c03666bb5d35dbb9R24-R45

…fix'

sebelga · 2023-05-03T06:41:14Z

x-pack/plugins/serverless_search/public/layout/nav.tsx

+            icon: 'logoEnterpriseSearch',
+          },
+        ]}
+        activeNavItemId="search_project_nav.root.overview"


Question: is this hardcoded string needed for now? Shouldn't we leave it empty until we receive this state from the Chrome service?

I left code in to set the activeNavItemId to search_project_nav.root. This allows the main "Search" bucket to be pre-expanded when the page is first loaded. Also, I added a comment stating how it's a temporary set-up.

sebelga · 2023-05-03T06:42:18Z

x-pack/plugins/serverless_search/public/layout/nav.tsx

+          },
+        ]}
+        activeNavItemId="search_project_nav.root.overview"
+        platformConfig={{}}


nit: Shouldn't we set platformConfig as optional if it requires an empty object?

…n/kibana into serverless/nav-component/search

elasticmachine · 2023-05-03T18:09:15Z

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

…#156526

…onent/search

…nent/search

kibana-ci · 2023-05-05T05:19:30Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: c309b1c

Failed CI Steps

FTR Configs #34

Test Failures

[job] [logs] FTR Configs #34 / expressions explorer emits an action and navigates

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`serverlessSearch`	4	28	+24

Page load bundle

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

id	before	after	diff
`serverlessSearch`	1.5KB	19.9KB	+18.4KB

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	19	21	+2
`securitySolution`	398	401	+3
total			+5

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	20	22	+2
`securitySolution`	478	481	+3
total			+5

History

💔 Build #125606 failed f11f607
💚 Build #125349 succeeded dcba408
💔 Build #125312 failed 12a8673
💔 Build #125279 failed 90c150a
💔 Build #125012 failed 5da6e09
💔 Build #125009 failed e62acc5f63778d81c95ea127bedfdf20bdcd54f6

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

sebelga

Code looks good (with some comments about i18n).

Tested locally and found a few UI glitches

When switching page the loading and icon are not in the same place and occupying the same space. This makes our Elastic logo jump around 😊 It'd be nice to set a fix width on the logo and lock its space. The loading should just swap in the same area and the logo should not move.

When navigating to some management > stack some links didn't point to the app and stayed on the welcome page. For example ILM (but there are more)

It seems that there is some issue with the page background color and the app background color. The following don't seem correct

sebelga · 2023-05-05T07:24:18Z

x-pack/plugins/serverless_search/public/layout/nav.tsx

+    title: '',
+    id: 'root',
+    items: [
+      { id: 'overview', title: 'Overview', href: '/app/enterprise_search/overview' },


I think we want to translate those title

sebelga · 2023-05-05T07:24:58Z

x-pack/plugins/serverless_search/public/layout/nav.tsx

+          {
+            id: 'search_project_nav',
+            items: navItems,
+            title: 'Search',


Here too title should be translated

sebelga · 2023-05-05T07:26:05Z

x-pack/plugins/serverless_search/public/layout/nav.tsx

+  },
+];
+
+export const createServerlessSearchSideNavComponent = (core: CoreStart) => () => {


We are inside the serverless_search plugin. I think we can simply call this createSideNavComponent

Dosant

lgtm,

in my opinion, the polishment from Seb's comment #156465 (review) could be done separately later and some of them are not for @elastic/appex-sharedux

Dosant · 2023-05-05T09:10:12Z

x-pack/plugins/serverless_search/public/plugin.ts

+    core: CoreStart,
    _startDeps: ServerlessSearchPluginStartDependencies
  ): ServerlessSearchPluginStart {
+    core.chrome.project.setSideNavComponent(createComponent(core));


I wonder if we should still set it through the serverless plugin as it was in the POC. not sure if there is a benefit to it

See: #156600 (comment)

I think we should bring it back

sebelga · 2023-05-05T13:08:48Z

in my opinion, the polishment from Seb's comment

@Dosant Links that don't work are not polishment IMO 😊 Is it due to our shared component, our navigation system? If we know the bug at least we should open an issue to fix it.

sphilipse · 2023-05-05T13:14:34Z

in my opinion, the polishment from Seb's comment

@Dosant Links that don't work are not polishment IMO 😊 Is it due to our shared component, our navigation system? If we know the bug at least we should open an issue to fix it.

We (as in: the Enterprise Search frontend team) will be reworking and iterating on all of these link over the coming weeks. Putting significant effort into getting them working in this PR is likely to be wasted effort. In fact, keeping this PR from merging over those links will actually end up slowing down the effort to get them fixed, as we can't really get to them before this is merged :)

tsullivan · 2023-05-05T16:13:18Z

Tested locally and found a few UI glitches

I will open separate issues for the UI glitches and non-working Platform links. Thanks for bringing those up!

tsullivan force-pushed the serverless/nav-component/search branch from e62acc5 to d7da5f0 Compare May 3, 2023 05:29

Add side navigation component for serverless search

0493ffb

tsullivan force-pushed the serverless/nav-component/search branch from d7da5f0 to 0493ffb Compare May 3, 2023 05:32

tsullivan commented May 3, 2023

View reviewed changes

tsullivan requested review from Dosant and sebelga May 3, 2023 05:34

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

5da6e09

…fix'

tsullivan added release_note:skip Skip the PR/issue when compiling release notes Team:SharedUX Platform AppEx-SharedUX (formerly Global Experience) t// Project:Serverless Work as part of the Serverless project for its initial release labels May 3, 2023

sebelga reviewed May 3, 2023

View reviewed changes

sebelga mentioned this pull request May 3, 2023

[ChromeNavPackage] Refactor to work with ChromeNavigationNode #156526

Merged

tsullivan and others added 2 commits May 3, 2023 10:11

simplify per code review

5b8f09d

Merge branch 'main' into serverless/nav-component/search

2fef742

tsullivan requested a review from clintandrewhall May 3, 2023 17:54

tsullivan added 3 commits May 3, 2023 11:04

rename create component function

cd1cad8

Re-introduce hardcoded activeNavItemId + comment

4199a7a

Merge branch 'serverless/nav-component/search' of github.com:tsulliva…

90c150a

…n/kibana into serverless/nav-component/search

tsullivan mentioned this pull request May 3, 2023

[SharedUx/POC] New package for project-style navigation. Integration from Serverless Search POC #153508

Closed

tsullivan marked this pull request as ready for review May 3, 2023 18:09

tsullivan requested a review from a team as a code owner May 3, 2023 18:09

bump serverlessSearch bundle size limit

12a8673

tsullivan requested a review from a team as a code owner May 3, 2023 19:48

jbudz approved these changes May 3, 2023

View reviewed changes

tsullivan and others added 4 commits May 3, 2023 14:43

re-bump the optimizer limit for serverless search plugin

897ffcf

Merge branch 'main' into serverless/nav-component/search

dcba408

Refactor shared-ux chrome nav package to work with ChromeNavigationNode

1980903

Fix jest test

00a3b5e

tsullivan added 3 commits May 3, 2023 14:56

[ChromeNavPackage] Refactor to work with ChromeNavigationNode elastic…

4945a14

…#156526

Merge branch 'chrome-nav/set-navigation-api' into serverless/nav-comp…

ad537f1

…onent/search

api changes for navigation component refactor

15be9ad

tsullivan requested review from TattdCodeMonkey and sphilipse May 4, 2023 14:35

TattdCodeMonkey approved these changes May 4, 2023

View reviewed changes

sphilipse approved these changes May 4, 2023

View reviewed changes

tsullivan and others added 3 commits May 4, 2023 11:04

Merge remote-tracking branch 'elastic/main' into serverless/nav-compo…

f11f607

…nent/search

re-bump the optimizer limit for serverless search plugin

e7bc9ba

Merge branch 'main' into serverless/nav-component/search

c309b1c

sebelga reviewed May 5, 2023

View reviewed changes

Dosant approved these changes May 5, 2023

View reviewed changes

Dosant mentioned this pull request May 5, 2023

[Security Solution][Serverless] Left navigation #156600

Merged

tsullivan merged commit 93d941b into elastic:main May 5, 2023

tsullivan deleted the serverless/nav-component/search branch May 5, 2023 16:13

kibanamachine added v8.9.0 backport:skip This PR does not require backporting labels May 5, 2023

kpatticha mentioned this pull request May 10, 2023

[Observability] Serverless navigation #153777

Closed

Conversation

tsullivan commented May 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Screenshots

Known issues

Checklist

Uh oh!

tsullivan May 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented May 3, 2023

Uh oh!

kibana-ci commented May 5, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Module Count

Page load bundle

ESLint disabled line counts

Total ESLint disabled count

History

Uh oh!

sebelga left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dosant left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebelga commented May 5, 2023

Uh oh!

sphilipse commented May 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tsullivan commented May 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

tsullivan commented May 3, 2023 •

edited

Loading

tsullivan May 3, 2023 •

edited

Loading

sphilipse commented May 5, 2023 •

edited

Loading