[Stateful sidenav] Dynamic links #183157

sebelga · 2024-05-10T14:11:44Z

In this PR I've added the dynamic links to the new solution view side navigation for the "enterprise search" solution.

I've decided to remove the original package created for the navigation tree declaration and move it to the enterprise_search plugin as it created an unnecessary indirection and boilerplate.

Fixes #179751

How to test

Add the following in the kibana.dev.yml

xpack.cloud.id: "ftr_fake_cloud_id:aGVsbG8uY29tOjQ0MyRFUzEyM2FiYyRrYm4xMjNhYmM="
xpack.cloud.base_url: "https://cloud.elastic.co"
xpack.cloud.deployment_url: "/deployments/deploymentId"

navigation.solutionNavigation.enabled: true
xpack.cloud_integrations.experiments.enabled: true
xpack.cloud_integrations.experiments.flag_overrides:
  "solutionNavEnabled": true

Screenshots

sebelga · 2024-05-10T14:13:17Z

/ci

sebelga · 2024-05-13T09:09:17Z

/ci

sebelga · 2024-05-13T12:32:59Z

/ci

sebelga · 2024-05-13T13:15:05Z

/ci

…fix'

sebelga · 2024-05-14T11:36:47Z

/ci

sebelga · 2024-05-14T18:00:08Z

/ci

x-pack/plugins/enterprise_search/public/applications/shared/layout/nav.tsx

elasticmachine · 2024-05-21T10:55:29Z

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

sebelga · 2024-05-21T10:55:56Z

/ci

sebelga · 2024-05-21T10:57:10Z

Thanks for the early review @TattdCodeMonkey ! 👍 I'd like to keep this PR focused only on the dynamic links and leave refactors to a separate PR if possible.
I've added tests and fixed CI, could you have another look? Thanks!

gsoldevila

Core changes LGTM (review only)

Dosant

code lgtm and tested a bit

Dosant · 2024-05-22T08:25:42Z

packages/shared-ux/chrome/navigation/src/ui/components/navigation_section_ui.tsx

-  const onClick = (e: React.MouseEvent) => {
+  const onClick = (e: React.MouseEvent<HTMLElement | HTMLButtonElement>) => {
+    if (customOnClick) {
+      customOnClick(e);


do we need to prevent default and/or stop propagation or is the idea that it has to be handled by the CB?

I see that below propagation is stopped even if customOnClick is passed

We don't want to prevent default for the customOnClick as the consumer handles it.
Regarding the stopPropagation calls, those were probably added to fix some behaviour but it works fine without them. I removed those in 891581c

Thanks for pointing that out 👍

Dosant · 2024-05-22T08:29:27Z

.../plugins/enterprise_search/public/applications/analytics/components/layout/page_template.tsx

+
+  useEffect(() => {
+    return () => {
+      updateSideNavDefinition({ collections: undefined });


I'm not sure why it is in a separate useEffect and not cleaned up on the previous one.

maybe the intention was to clean up only on unmount? but then the dependencies need to be [updateSideNavDefinition]

I put it in a separate useEffect as I only want updateSideNavDefinition to be called with undefined when the comp unmounts. Otherwise we would get 2 calls each time navItems changes, right? Or does React handles this case and merges the 2 calls into 1?

Great catch for the useEffect dependency, I removed it in 3271e19

React should handle this and only call this function a single time when the component is unmounted.

Apparently it does have a slight effect: it calls the cleaning function on more time

Everytime navItems changes

Only when component unmounts

I am going to leave it as is for now if that's OK.

kibana-ci · 2024-05-22T11:43:54Z

💚 Build Succeeded

Buildkite Build
Commit: d27b060

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`enterpriseSearch`	2320	2321	+1
`navigation`	88	86	-2
total			-1

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`@kbn/solution-nav-es`	6	-	-6
`navigation`	47	48	+1
total			-5

Async chunks

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

id	before	after	diff
`enterpriseSearch`	2.7MB	2.7MB	+6.8KB
`navigation`	22.0KB	22.3KB	+252.0B
`serverless`	22.3KB	22.5KB	+252.0B
total			+7.3KB

Page load bundle

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

id	before	after	diff
`core`	409.8KB	409.9KB	+129.0B
`enterpriseSearch`	48.5KB	48.9KB	+403.0B
`navigation`	31.1KB	27.6KB	-3.6KB
total			-3.0KB

Unknown metric groups

API count

id	before	after	diff
`@kbn/core-chrome-browser`	205	206	+1
`@kbn/solution-nav-es`	6	-	-6
`navigation`	49	50	+1
total			-4

async chunk count

id	before	after	diff
`enterpriseSearch`	16	17	+1

ESLint disabled in files

id	before	after	diff
`enterpriseSearch`	1	2	+1

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	20	21	+1

History

💚 Build #211271 succeeded f2590cf
💔 Build #211207 failed 51f544a
💔 Build #211191 failed 67fca04
💔 Build #211050 failed 774e214
💔 Build #211016 failed e65f4e8
💔 Build #210984 failed f8f7526

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

cc @sebelga

TattdCodeMonkey · 2024-05-22T13:50:10Z

.../plugins/enterprise_search/public/applications/analytics/components/layout/page_template.tsx

+
+  useEffect(() => {
+    return () => {
+      updateSideNavDefinition({ collections: undefined });


React should handle this and only call this function a single time when the component is unmounted.

x-pack/plugins/enterprise_search/public/applications/shared/layout/nav.tsx

sebelga and others added 12 commits May 14, 2024 10:02

Move search nav definition to plugin

839663b

Don't add the search default definition

3b9bc09

Remove search solution nav package

9b300ff

Sort keys ascending for linting

71a47e9

Set the correct breadcrumb root even when no active tree yet

e496442

Add nav tree definition from es plugin

e5e29e1

Allow onClick to be passed for a navigation node

62629cc

Add handler to update the side nav definition

226f757

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

d0d6471

…fix'

[CI] Auto-commit changed files from 'node scripts/generate codeowners'

19f32d8

Fix i18n issue

2235919

Fix TS issue & update jest snapshot

d1373e1

sebelga force-pushed the stateful-sidenav/dynamic-links branch from e69d849 to d1373e1 Compare May 14, 2024 09:18

Add dynamic links for search applications

894f7d3

sebelga added 5 commits May 14, 2024 15:52

Always return nav items when chrome style is project

260ffbe

Add dynamic links for collections

6c38c1b

Update NavigationSectionUI to better handle accordion states

346ec18

Add comments

17fbdc3

Fix TS issues

0c9f375

sebelga commented May 15, 2024

View reviewed changes

x-pack/plugins/enterprise_search/public/applications/shared/layout/nav.tsx Show resolved Hide resolved

sebelga added 4 commits May 15, 2024 11:19

Update comments

eec7e67

Add tests for isActiveFromUrl

da4c2a4

Refactors

cae3e0c

Refactor navigation section UI

462d98f

Load navigation tree async

a4f2578

sebelga self-assigned this May 21, 2024

sebelga added release_note:skip Skip the PR/issue when compiling release notes Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) Feature:Chrome Core's Chrome UI (sidenav, header, breadcrumbs) labels May 21, 2024

sebelga added 4 commits May 21, 2024 11:29

Cleanup

d932274

Refactor

3be9f6e

Update comments

ca28657

Add jest test for custom onClick

67fca04

sebelga marked this pull request as ready for review May 21, 2024 10:55

sebelga requested review from a team as code owners May 21, 2024 10:55

sebelga requested a review from TattdCodeMonkey May 21, 2024 10:55

sebelga added 2 commits May 21, 2024 12:56

Merge branch 'main' into stateful-sidenav/dynamic-links

51f544a

Merge branch 'main' into stateful-sidenav/dynamic-links

f2590cf

gsoldevila approved these changes May 21, 2024

View reviewed changes

Dosant approved these changes May 22, 2024

View reviewed changes

sebelga added 3 commits May 22, 2024 11:22

Remove stopPropagation calls

891581c

Remove unnecessary useEffect dependency

3271e19

Merge branch 'main' into stateful-sidenav/dynamic-links

d27b060

TattdCodeMonkey approved these changes May 22, 2024

View reviewed changes

sebelga merged commit 5b38b5a into elastic:main May 22, 2024
34 checks passed

sebelga deleted the stateful-sidenav/dynamic-links branch May 22, 2024 15:55

kibanamachine added v8.15.0 backport:skip This commit does not require backporting labels May 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Stateful sidenav] Dynamic links #183157

[Stateful sidenav] Dynamic links #183157

sebelga commented May 10, 2024 •

edited

sebelga commented May 10, 2024

sebelga commented May 13, 2024

sebelga commented May 13, 2024

sebelga commented May 13, 2024

sebelga commented May 14, 2024

sebelga commented May 14, 2024

elasticmachine commented May 21, 2024

sebelga commented May 21, 2024

sebelga commented May 21, 2024

gsoldevila left a comment

Dosant left a comment

Dosant May 22, 2024

sebelga May 22, 2024

Dosant May 22, 2024

sebelga May 22, 2024

TattdCodeMonkey May 22, 2024

sebelga May 22, 2024 •

edited

kibana-ci commented May 22, 2024

API count

async chunk count

ESLint disabled in files

Total ESLint disabled count

TattdCodeMonkey May 22, 2024

[Stateful sidenav] Dynamic links #183157

[Stateful sidenav] Dynamic links #183157

Conversation

sebelga commented May 10, 2024 • edited

How to test

Screenshots

sebelga commented May 10, 2024

sebelga commented May 13, 2024

sebelga commented May 13, 2024

sebelga commented May 13, 2024

sebelga commented May 14, 2024

sebelga commented May 14, 2024

elasticmachine commented May 21, 2024

sebelga commented May 21, 2024

sebelga commented May 21, 2024

gsoldevila left a comment

Choose a reason for hiding this comment

Dosant left a comment

Choose a reason for hiding this comment

Dosant May 22, 2024

Choose a reason for hiding this comment

sebelga May 22, 2024

Choose a reason for hiding this comment

Dosant May 22, 2024

Choose a reason for hiding this comment

sebelga May 22, 2024

Choose a reason for hiding this comment

TattdCodeMonkey May 22, 2024

Choose a reason for hiding this comment

sebelga May 22, 2024 • edited

Choose a reason for hiding this comment

Everytime navItems changes

Only when component unmounts

kibana-ci commented May 22, 2024

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Page load bundle

API count

async chunk count

ESLint disabled in files

Total ESLint disabled count

History

TattdCodeMonkey May 22, 2024

Choose a reason for hiding this comment

sebelga commented May 10, 2024 •

edited

sebelga May 22, 2024 •

edited

Everytime `navItems` changes