Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SharedUxChromeNavigation] Use deeplink id instead of href #159125

Merged
merged 66 commits into from
Jun 13, 2023

Conversation

sebelga
Copy link
Contributor

@sebelga sebelga commented Jun 6, 2023

In this PR I removed the use of href for the serverless side navigation definition and replace them with link which points to either the root of an app or a deeplink id.

Autocomplete

Now that all the deeplinks types are exposed, TS autocomplete is available when building a side navigation.

Screenshot 2023-06-12 at 13 18 32 Screenshot 2023-06-12 at 13 19 06

 href support

It is still possible to pass an href to a navigation node definition but only for external links. For internal link a deeplink id must be used. If an external href is added in the navigation tree, it is marked as external in the UI

Screenshot 2023-06-12 at 13 12 28

Packages created

Instead of creating a separate package for each of the app and its deeplinks I've decided to group the deeplink by domain, the same way we group them in the navigation. It is thus expected that there will be multiple team owning those packages. As we don't plan to add frequently new deeplinks I think that the trade off of fewer packages with a bit more "noise" on PR ownership should be OK.

### Deeplinks

  • @kbn/deeplinks-devtools
  • @kbn/deeplinks-analytics
  • @kbn/deeplinks-ml
  • @kbn/deeplinks-management
  • @kbn/deeplinks-search
  • @kbn/deeplinks-observability

Navigations "presets"

It is possible to add default navigation for certain domain in the navigation tree by simply loading a "preset".

// by config
{
  type: 'navGroup',
  preset: 'analytics',
},

// or using React component
<Navigation.Group preset="analytics" defaultIsCollapsed={false} />

Those "default" are now owned by the teams owning those domain. If we don't feel the need to expose a "default" navigation for a certain domain we can remove the preset and its corresponding package.

  • @kbn/default-nav-analytics
  • @kbn/default-nav-ml
  • @kbn/default-nav-devtools
  • @kbn/default-nav-management

Fixes #158658
Fixes #158376

@sebelga sebelga force-pushed the sharedux-chrome-nav/deeplinks-packages branch from a05679e to 92784b8 Compare June 9, 2023 10:36
sebelga and others added 29 commits June 12, 2023 13:17
@kpatticha
Copy link
Contributor

kpatticha commented Jun 13, 2023

@gbamparop I looked into it and the Observability links you mentioned are marked as hidden from navigation that's why they don't appear.

Screenshot 2023-06-13 at 09 32 23 Screenshot 2023-06-13 at 09 33 36 Screenshot 2023-06-13 at 09 31 55
If it is an easy fix I can change that boolean where you tell me, otherwise I think the best is that the observability team takes it from here (once the PR is merged) and make sure all the deeplink they require have their app mounted and the deeplink is not marked as hidden.

Regarding the "Logs" link, in main it currently points to "discover" app which is obviously wrong. Now it does not appear probably because of this condition https://github.com/elastic/kibana/blob/main/x-pack/plugins/infra/public/plugin.ts#L194

For the Machine learning sublinks, the "ml" team is responsible of the default navigation and will probably update it once this PR is merged (cc @jgowdyelastic)

so it seems we need to update

but wondering why on serverless it works differently from the classic Kibana?

@sebelga
Copy link
Contributor Author

sebelga commented Jun 13, 2023

wondering why on serverless it works different from the classic Kibana

Yeah it is strange. From the file you point it seems that deeplink are active. Something else is disabling them but not seeing any updater$ being registered to update deeplinks

@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Jun 13, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:APM)

@sebelga
Copy link
Contributor Author

sebelga commented Jun 13, 2023

@kpatticha

So it seems that by default deeplinks are hidden (https://github.com/elastic/kibana/blob/main/packages/core/application/core-application-browser-internal/src/utils/get_app_info.ts#L47) and you need to manually set it to visible.

I've done it for the "Apm" services (7686eb2)

For the logs, it is probably something like this (https://github.com/elastic/kibana/blob/main/x-pack/plugins/infra/public/plugin.ts#L178) that hides the link in the side nav. You will have to see with the infra team how to enable them in serverless for Observability.

@sebelga sebelga enabled auto-merge (squash) June 13, 2023 14:35
@sebelga sebelga disabled auto-merge June 13, 2023 14:36
@sebelga
Copy link
Contributor Author

sebelga commented Jun 13, 2023

@gbamparop I had to revert my change to mark the deeplink as visible as it was failing a test "APM specs feature controls security global apm all privileges shows apm navlink".

I think it is better that you mark the link(s) as visible in a following PR if they are meant to be seen in the navigation.

@sebelga sebelga requested a review from gbamparop June 13, 2023 16:01
@sebelga sebelga enabled auto-merge (squash) June 13, 2023 16:02
@sebelga sebelga merged commit fb41ca5 into elastic:main Jun 13, 2023
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
devTools 25 27 +2
serverlessObservability 40 45 +5
serverlessSearch 84 89 +5
total +12

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/core-chrome-browser 55 58 +3
@kbn/deeplinks-analytics - 5 +5
@kbn/deeplinks-devtools - 5 +5
@kbn/deeplinks-management - 4 +4
@kbn/deeplinks-ml - 3 +3
@kbn/deeplinks-observability - 3 +3
@kbn/deeplinks-search - 3 +3
@kbn/default-nav-analytics - 7 +7
@kbn/default-nav-devtools - 7 +7
@kbn/default-nav-management - 7 +7
@kbn/default-nav-ml - 7 +7
@kbn/shared-ux-chrome-navigation 46 44 -2
total +52

Any counts in public APIs

Total count of every any typed public API. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats any for more detailed information.

id before after diff
@kbn/default-nav-analytics - 1 +1
@kbn/default-nav-devtools - 1 +1
@kbn/default-nav-management - 1 +1
@kbn/default-nav-ml - 1 +1
total +4

Async chunks

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

id before after diff
ml 3.4MB 3.4MB +604.0B

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
@kbn/deeplinks-management - 2 +2
@kbn/shared-ux-chrome-navigation 7 5 -2
total -0

Page load bundle

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

id before after diff
devTools 10.7KB 10.9KB +169.0B
ml 73.9KB 73.9KB +14.0B
serverlessObservability 26.1KB 25.2KB -912.0B
serverlessSearch 30.1KB 28.5KB -1.6KB
total -2.3KB
Unknown metric groups

API count

id before after diff
@kbn/core-chrome-browser 141 150 +9
@kbn/deeplinks-analytics - 5 +5
@kbn/deeplinks-devtools - 5 +5
@kbn/deeplinks-management - 4 +4
@kbn/deeplinks-ml - 3 +3
@kbn/deeplinks-observability - 3 +3
@kbn/deeplinks-search - 3 +3
@kbn/default-nav-analytics - 7 +7
@kbn/default-nav-devtools - 7 +7
@kbn/default-nav-management - 7 +7
@kbn/default-nav-ml - 7 +7
@kbn/shared-ux-chrome-navigation 76 68 -8
total +52

ESLint disabled line counts

id before after diff
@kbn/shared-ux-chrome-navigation 0 4 +4
enterpriseSearch 13 15 +2
securitySolution 409 413 +4
total +10

Total ESLint disabled count

id before after diff
@kbn/shared-ux-chrome-navigation 0 4 +4
enterpriseSearch 14 16 +2
securitySolution 492 496 +4
total +10

History

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

cc @sebelga

@kibanamachine kibanamachine added v8.9.0 backport:skip This commit does not require backporting labels Jun 13, 2023
@sebelga sebelga deleted the sharedux-chrome-nav/deeplinks-packages branch June 13, 2023 17:35
@gbamparop
Copy link
Contributor

@gbamparop I had to revert my change to mark the deeplink as visible as it was failing a test "APM specs feature controls security global apm all privileges shows apm navlink".

I think it is better that you mark the link(s) as visible in a following PR if they are meant to be seen in the navigation.

Thanks for looking into this, we'll create a follow-up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Project:Serverless MVP Project:Serverless Work as part of the Serverless project for its initial release release_note:skip Skip the PR/issue when compiling release notes Team:APM All issues that need APM UI Team support Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) v8.9.0
Projects
None yet