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] fix discover breadcrumbs missing search title #163607

Merged

Conversation

Dosant
Copy link
Contributor

@Dosant Dosant commented Aug 10, 2023

Summary

Aaddress #163337 for discover saved search

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.

Screenshot 2023-08-10 at 15 22 08

@Dosant Dosant changed the title discover serverless breadcrumbs [Serverless] fix discover breadcrumbs missing search title Aug 10, 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 10, 2023
@Dosant Dosant marked this pull request as ready for review August 10, 2023 14:21
@Dosant Dosant requested a review from a team as a code owner August 10, 2023 14:21
@elasticmachine
Copy link
Contributor

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

@Dosant Dosant self-assigned this Aug 10, 2023
Copy link
Contributor

@davismcphee davismcphee left a comment

Choose a reason for hiding this comment

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

Tested locally and it looks to be working now, thanks! Would you mind adding a test for it though? Either a simple Jest test for discover_main_route.tsx to check that serverless.setBreadcrumbs is called or a Serverless functional test would be valuable IMO.

@Dosant
Copy link
Contributor Author

Dosant commented Aug 11, 2023

Thanks @davismcphee, I've tried to add a unit test as you suggested, but I thought it was too much friction to mock all the needed states, and the tests for the whole component were too slow for adding a lot of separate breadcrumbs test cases.

Instead, I decided to conslidate all the setBreadcrumb calls in a single function and unit test that function. Hope this is fine.

Copy link
Contributor

@davismcphee davismcphee left a comment

Choose a reason for hiding this comment

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

Instead, I decided to conslidate all the setBreadcrumb calls in a single function and unit test that function. Hope this is fine.

That works for me, and the setBreadcrumbs function is a nice cleanup over the old approach anyway. Tested again and it's working as expected, LGTM and thanks for the fix + tests 👍

On a related note (although not due to this PR since it happens in main too), I just realized in testing again that when clicking the root Discover breadcrumb from the single doc or surrounding docs view, the Discover context is lost (e.g. filters, query, saved search, etc.). It looks like the breadcrumb always points to /app/discover#/ which clears our URL state and causes the lost context.

I can create a separate issue for this, but do you know if this is something that can be dealt with from the Discover side or something the Serverless nav code will have to handle?

@Dosant
Copy link
Contributor Author

Dosant commented Aug 14, 2023

I just realized in testing again that when clicking the root Discover breadcrumb from the single doc or surrounding docs view, the Discover context is lost (e.g. filters, query, saved search, etc.). It looks like the breadcrumb always points to /app/discover#/ which clears our URL state and causes the lost context.

In serverless nav there is currently no way to do this by changing the href, I think it falls under #163488, I will add the use-case.
As a workaround, we could also consider keeping the state in-memory and then restore it

@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
discover 557.4KB 557.4KB +15.0B

Page load bundle

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

id before after diff
discover 31.4KB 31.5KB +24.0B

History

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

cc @Dosant

@Dosant Dosant merged commit 933f2a5 into elastic:main Aug 14, 2023
@kibanamachine kibanamachine added v8.10.0 backport:skip This commit does not require backporting labels Aug 14, 2023
Dosant added a commit that referenced this pull request Oct 27, 2023
## Summary

This changes how serverless breadcrumbs are implemented in Discover. It
simplifies the current implementation and also fixes an issue with
"Discover" breadcrumb missing context from it's URL (see
#163607 (review))

Instead of using `serverless.setBreadcrums` API for deeper context
serverless breadcrumb, this PR adds `deepLinkId` to base discover
breadcrumb. Based on this id the navigational breadcrumbs are merged
with the regular breadcrumbs set in `chrome.setBreadcrumbs`). (This
feature was implemented here:
#169513)

This PR also improves "Discover" breadcrumb in oblt project when
navigated from logs explorer to discover by adding "discover" to the
navigation tree and hiding it from side nav.

![Screenshot 2023-10-25 at 13 34
05](https://github.com/elastic/kibana/assets/7784120/19bbd1a8-809e-429a-a0cf-0bc0f22f49d5)
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.

5 participants