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

[Serverless] Partially fix lens/maps/visualize breadcrumbs missing title #163476

Merged
merged 6 commits into from
Aug 11, 2023

Conversation

Dosant
Copy link
Contributor

@Dosant Dosant commented Aug 9, 2023

Summary

Partially address #163337 for lens, visualize, maps

Context:

In serverless navigation, we changed how breadcrumbs work. Instead of setting the full path manually, we automatically calculate the main parts of the path from the side nav + current URL. This was done to keep side nav and breadcrumbs in sync as much as possible and solve consistency issues with breadcrumbs across apps.
https://docs.elastic.dev/kibana-dev-docs/serverless-project-navigation#breadcrumbs

Apps can append custom deeper context using the serverless.setBreadcrumbs API. Regular core.chrome.setBreadcrumbs has no effect when the serverless nav is rendered.

Fix

This PR fixes lens, visualize, and maps to add "title" breadcrumb in serverless. Unfortunately, it doesn't fully restore the full breadcrumbs functionality visualize/maps/lens have in the non-serverless Kibana:

In the non-serverless Kibana lens/visualize/maps have sophisticated breadcrumbs where context takes into account ByValue and originatingApp and can switch depending on the context. For example, if the user is coming from "Dashboard" to edit "byValue" Lens visualization, Lens breadcrumbs display "Dashboard > Create", instead of "Visualization > Create".

Currently, we can't repeat this behavior with serverless breadcrumbs because the context is set by the navigation config, e.g.:

getIsActive: ({ pathNameSerialized, prepend }) => {
return (
pathNameSerialized.startsWith(prepend('/app/visualize')) ||
pathNameSerialized.startsWith(prepend('/app/lens')) ||
pathNameSerialized.startsWith(prepend('/app/maps'))
);

In this PR I attempt to do a quick fix for the serverless breadcrumbs by simply appending the last ("title") part of the breadcrumb. In a follow up we need to think about how to bring back the original breadcrumbs functionality with changing Visualize <-> Dashboard context. We also will need to figure out how to sync the changing context with the side nav, as we don't want to show "Dashboard" in the breadcrumb, but have "Visualization" highlighted in the side nav. Here is the issue: #163488

Alternative

Another quick alternative is to force the same breadcrumbs in serverless as we have in non-serverless. But, in this case, we would lose synchronization between the side nav and the breadcrumbs, here are some examples:

Editing saved viz from visualize:
Screenshot 2023-08-09 at 12 19 44

Editing panel from a dashboard:
Screenshot 2023-08-09 at 12 17 35

I decided to go with the first the simpler option, where, for now, we just change the last breadcrumb in serverless and we'll discuss how to bring back the original behavior here #163488

@Dosant Dosant changed the title [Serverless] Partially fix lens breadcrumbs missing title [Serverless] Partially fix lens/maps/visualize breadcrumbs missing title Aug 9, 2023
@Dosant Dosant added release_note:skip Skip the PR/issue when compiling release notes Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) Project:Serverless Work as part of the Serverless project for its initial release Feature:Chrome Core's Chrome UI (sidenav, header, breadcrumbs) labels Aug 9, 2023
@Dosant Dosant marked this pull request as ready for review August 9, 2023 15:09
@Dosant Dosant requested review from a team as code owners August 9, 2023 15:09
@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kibana-gis changes LGTM
code review only

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works fine, changes LGTM! I tested serverless and on prem in Chrome and breadcrumbs work as expected

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
lens 1.4MB 1.4MB +102.0B
maps 2.8MB 2.8MB +139.0B
visualizations 262.8KB 263.5KB +721.0B
total +962.0B

Page load bundle

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

id before after diff
maps 45.3KB 45.3KB +56.0B
visualizations 56.8KB 56.8KB +24.0B
total +80.0B

History

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

cc @Dosant

@Dosant Dosant merged commit e944a19 into elastic:main Aug 11, 2023
@kibanamachine kibanamachine added v8.10.0 backport:skip This commit does not require backporting labels Aug 11, 2023
jloleysens added a commit to jloleysens/kibana that referenced this pull request Aug 11, 2023
* main: (64 commits)
  [ML] Transforms: Fix privileges check. (elastic#163687)
  [Log Explorer] Add test suite for Dataset Selector (elastic#163079)
  [Security Solution][Endpoint] Add API checks to Endpoint Policy create/update for checking `endpointPolicyProtections` is enabled (elastic#163429)
  [Security Solution] Fix flaky test: x-pack/test/detection_engine_api_integration/security_and_spaces/update_prebuilt_rules_package/update_prebuilt_rules_package·ts - update_prebuilt_rules_package should allow user to install prebuilt rules from scratch, then install new rules and upgrade existing rules from the new package (elastic#163241)
  [Security Solution] expandable flyout - replace feature flag with advanced settings toggle (elastic#161614)
  [DOCS] Adds the release notes for the 8.9.1 release. (elastic#163578)
  [FTR] Implement browser network condition utils (elastic#163633)
  [Security Solution] Unskip rules table auto-refresh Cypress tests (elastic#163451)
  [Security Solution] Re-enable fixed rule snoozing Cypress test (elastic#160037)
  [Flaky Test elastic#111821] Mock `moment` to avoid midnight TZ issues (elastic#163157)
  Document interactive setup (elastic#163619)
  [Lens] Align decoration color with text color for layer actions (elastic#163630)
  [Lens] Relax counter field checks for saved visualizations with unsupported operations (elastic#163515)
  [Security Solution][Endpoint] Removes pMap and uses a for loop instead (elastic#163509)
  [Enterprise Search] Update Workplace Search connectors doclink (elastic#163676)
  Update APM (main) (elastic#163623)
  [Serverless] Partially fix lens/maps/visualize breadcrumbs missing title  (elastic#163476)
  [Flaky elastic#118272] Unskip tests (elastic#163319)
  [APM] Make service group saved objects exportable (elastic#163569)
  [Observability AI Assistant] Action menu item (elastic#163463)
  ...
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 Feature:Chrome Core's Chrome UI (sidenav, header, breadcrumbs) 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:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) v8.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants