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.

Sign up for GitHub

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

Jump to bottom

[Stateful sidenav] Fix collapsed menu for panels with no landing pages #195904

Merged
merged 7 commits into from
Oct 14, 2024
Merged

[Stateful sidenav] Fix collapsed menu for panels with no landing pages #195904

merged 7 commits into from
Oct 14, 2024

Conversation

sebelga
Copy link
Contributor

@sebelga sebelga commented Oct 11, 2024 •
edited by kibanamachine
Loading

In this PR I've updated the logic of the side navigation to render the "panel opener groups" as "accordions" whenever the side nav is collapsed, **only for groups that don't have a link (landing page)". This change does not impact the security navigation which has a landing page for each of the panels.

When the side nav is collapsed, a popover is used to render the navigation. We can't open the panel in that mode as the panel renders beneath the popover.

How to test

  • Add the following in your kibana.dev.yml
xpack.cloud.id: "ftr_fake_cloud_id:aGVsbG8uY29tOjQ0MyRFUzEyM2FiYyRrYm4xMjNhYmM="
xpack.cloud.base_url: "https://cloud.elastic.co"
  • Launch Kibana and set the "observability" solution on the default space
  • Test the navigation when the side nav is expanded or collapsed

Screenshot

Collapsed

Screenshot 2024-10-11 at 13 11 55

Extended

Screenshot 2024-10-11 at 13 11 42

sebelga
sebelga commented Oct 11, 2024
View reviewed changes
packages/shared-ux/chrome/navigation/src/services.tsx
@@ -44,7 +44,6 @@ export const NavigationKibanaProvider: FC<PropsWithChildren<NavigationKibanaDepe
basePath,
recentlyAccessed$: chrome.recentlyAccessed.get$(),
navigateToUrl,
navIsOpen: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did some cleaning, this navIsOpen is not used anywhere.

@sebelga sebelga marked this pull request as ready for review October 11, 2024 12:26
@sebelga sebelga requested review from a team as code owners October 11, 2024 12:26
@sebelga sebelga self-assigned this Oct 11, 2024
@sebelga sebelga added release_note:skip Skip the PR/issue when compiling release notes Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) Feature:Chrome Core's Chrome UI (sidenav, header, breadcrumbs) labels Oct 11, 2024
@elasticmachine
Copy link
Contributor

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

@mgiota mgiota self-requested a review October 11, 2024 12:37
mgiota
mgiota approved these changes Oct 11, 2024
View reviewed changes
Copy link
Contributor

@mgiota mgiota left a comment

Choose a reason for hiding this comment

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

I tested it and works fine! When sidenav is collapsed, o11y renders panel opener groups as accordions for groups that don't have landing pages.

Screen.Recording.2024-10-11.at.15.42.52.mov

I verify that this change does not change security.

I was curious to see the implementation. I have thought about somehow getting directly in the o11y navigation_tree the information isSideNavCollapsed and conditionally rendering items as accordion vs panelOpener. But I see you did the changes directly to the navigation package, which is fine!

@botelastic botelastic bot added ci:project-deploy-observability Create an Observability project Team:obs-ux-management Observability Management User Experience Team labels Oct 11, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/obs-ux-management-team (Team:obs-ux-management)

@sebelga
Copy link
Contributor Author

sebelga commented Oct 11, 2024

Thanks for the review @mgiota !

I have thought about somehow getting directly in the o11y navigation_tree the information isSideNavCollapsed and conditionally rendering items as accordion vs panelOpener. But I see you did the changes directly to the navigation package, which is fine!

We want the same behaviour for any panel opener that does not have a landing page, so yes I fixed it globally 😊

@elasticmachine
Copy link
Contributor

elasticmachine commented Oct 13, 2024 •
edited
Loading

💚 Build Succeeded

Metrics [docs]

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/shared-ux-chrome-navigation 30 29 -1

Async chunks

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

id before after diff
navigation 25.3KB 25.6KB +377.0B
observability 467.5KB 467.4KB -23.0B
serverless 25.9KB 26.3KB +376.0B
total +730.0B
Unknown metric groups

API count

id before after diff
@kbn/shared-ux-chrome-navigation 39 38 -1

History

cc @sebelga

eokoneyo
eokoneyo approved these changes Oct 14, 2024
View reviewed changes
Copy link
Contributor

@eokoneyo eokoneyo left a comment

Choose a reason for hiding this comment

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

Verified locally, it works as advertised. I however noticed that on clicking a nav menu item within the popover, the popover remains visible even after the route has transitioned, wdyt?

@sebelga
Copy link
Contributor Author

sebelga commented Oct 14, 2024

Thanks for the review @eokoneyo ! 👍

I however noticed that on clicking a nav menu item within the popover, the popover remains visible even after the route has transitioned, wdyt?

That's the behaviour coming from the underlying EUI component. It has its pros and cons.
The pro is in case you are not sure which exact link you need to click, or if you are exploring the nav and want to "click around" to see what is available. You don't need to first open the navigation popover.
The con is that you need to click outside to close it.

Let's leave it like that for now and we can revisit if users complain.

@sebelga sebelga merged commit a3289e4 into elastic:main Oct 14, 2024
27 checks passed
@sebelga sebelga deleted the stateful-sidenav/fix-collapsed-menu-panels branch October 14, 2024 10:37
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11325829039

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 14, 2024
@sebelga
[Stateful sidenav] Fix collapsed menu for panels with no landing pages (
d5a5d93 
elastic#195904)

(cherry picked from commit a3289e4)
@kibanamachine kibanamachine mentioned this pull request Oct 14, 2024
Merged
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
✅ 8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

@kibanamachine kibanamachine mentioned this pull request Oct 14, 2024
Merged
kibanamachine added a commit that referenced this pull request Oct 14, 2024
@kibanamachine @sebelga
[8.x] [Stateful sidenav] Fix collapsed menu for panels with no landin…
8481b66 
…g pages (#195904) (#196098)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[Stateful sidenav] Fix collapsed menu for panels with no landing
pages (#195904)](#195904)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Sébastien
Loix","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-10-14T10:37:15Z","message":"[Stateful
sidenav] Fix collapsed menu for panels with no landing pages
(#195904)","sha":"a3289e440ad88825b1ae17495caf13ef5720ae7c","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","Team:SharedUX","backport:prev-minor","Feature:Chrome","ci:project-deploy-observability","Team:obs-ux-management"],"title":"[Stateful
sidenav] Fix collapsed menu for panels with no landing
pages","number":195904,"url":"https://github.com/elastic/kibana/pull/195904","mergeCommit":{"message":"[Stateful
sidenav] Fix collapsed menu for panels with no landing pages
(#195904)","sha":"a3289e440ad88825b1ae17495caf13ef5720ae7c"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/195904","number":195904,"mergeCommit":{"message":"[Stateful
sidenav] Fix collapsed menu for panels with no landing pages
(#195904)","sha":"a3289e440ad88825b1ae17495caf13ef5720ae7c"}}]}]
BACKPORT-->

Co-authored-by: Sébastien Loix <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eokoneyo eokoneyo eokoneyo approved these changes

@mgiota mgiota mgiota approved these changes

Assignees

@sebelga sebelga

Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) ci:project-deploy-observability Create an Observability project Feature:Chrome Core's Chrome UI (sidenav, header, breadcrumbs) release_note:skip Skip the PR/issue when compiling release notes Team:obs-ux-management Observability Management User Experience Team Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) v8.16.0 v9.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sebelga @elasticmachine @kibanamachine @mgiota @eokoneyo