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

NavigationItemOpenPanel: remove handling of landing page ("four squares" design) #210893

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

NavigationItemOpenPanel: remove handling of landing page ("four squares" design) #210893

wants to merge 17 commits into from
+87 −243

Conversation

tsullivan
Copy link
Member

@tsullivan tsullivan commented Feb 12, 2025 •
edited
Loading

Summary

Part of Epic: https://github.com/elastic/kibana-team/issues/1439
Requires: #212903

Changes:

  1. Moves the Solution Side Nav away from the "four squares" design pattern: where clicking the item label opens a landing page and the item icon opens the secondary nav panel. This was a custom component implemented in the Kibana package, not part of the EUI EuiCollapsibleNavBeta component.
  2. Changes some usage of @emotion/css to @emotion/react for better developer experience

Screenshots

Before

01-security-solution-before

After

02-security-solution-after

Checklist

Check the PR satisfies following conditions.

Reviewers should verify this PR satisfies this list as well.

  • Unit or functional tests were updated or added to match the most common scenarios
  • The PR description includes the appropriate Release Notes section, and the correct release_note:* label is applied per the guidelines

Identify risks

Does this PR introduce any risks? For example, consider risks like hard to test bugs, performance regression, potential of data loss.

Describe the risk, its severity, and mitigation for each identified risk. Invite stakeholders and evaluate how to proceed before merging.

  • This design pattern was only used in Security Solution. There is a small risk of regression issues in Security Solution navigation. This was mitigated by manual testing during development.

@tsullivan tsullivan added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) labels Feb 12, 2025
@tsullivan tsullivan requested a review from semd February 12, 2025 17:15
@tsullivan tsullivan self-assigned this Feb 12, 2025
@tsullivan tsullivan force-pushed the solution-nav/ignore-link-when-renderas-panel branch 7 times, most recently from 90e6ab1 to e072275 Compare February 18, 2025 20:20
@tsullivan tsullivan force-pushed the solution-nav/ignore-link-when-renderas-panel branch from e072275 to 099e52f Compare February 18, 2025 22:21
@tsullivan tsullivan marked this pull request as ready for review February 19, 2025 00:07
@tsullivan tsullivan requested review from a team as code owners February 19, 2025 00:07
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

@elasticmachine
Copy link
Contributor

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

tsullivan
tsullivan commented Feb 19, 2025
View reviewed changes
...packages/shared/shared-ux/chrome/navigation/src/ui/components/navigation_item_open_panel.tsx
const isExpanded = selectedNode?.path === path;
const isActive = hasLandingPage ? isActiveFromUrl(item.path, activeNodes) : isExpanded;
Copy link
Member Author

Choose a reason for hiding this comment

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

Previous code had a bug: isActiveFromUrl was only called if the nav item had a landing page. This was broken for the parent nav item buttons that are remaining in this PR.

@botelastic botelastic bot added the ci:project-deploy-observability Create an Observability project label Feb 19, 2025
@github-actions GitHub Actions
Copy link
Contributor

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • run docs-build : Re-trigger the docs validation. (use unformatted text in the comment!)

dmlemeshko
dmlemeshko approved these changes Feb 19, 2025
View reviewed changes
Copy link
Member

@dmlemeshko dmlemeshko left a comment

Choose a reason for hiding this comment

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

Changes in x-pack/test_serverless/functional/services/ml LGTM

@semd
Copy link
Contributor

semd commented Feb 20, 2025 •
edited
Loading

@tsullivan The code looks good, but with these changes we lose access to the custom Dashboards functionality (dashboards "landing" page). Also, not all Assets links appear in its secondary panel.

I am talking with our PM to see how we can handle that.
What release are we targeting this?

@tsullivan tsullivan added the ci:project-deploy-elasticsearch Create an Elasticsearch Serverless project label Feb 20, 2025
@tsullivan
Copy link
Member Author

@tsullivan The code looks good, but with these changes we lose access to the custom Dashboards functionality (dashboards "landing" page). Also, not all Assets links appear in its secondary panel.

@semd Understood. If we need to add changes to the Security Solution navigation tree in this PR, let's make sure we do that. I will probably need your guidance on where to add the code. Would you be able to send me a PR to this branch after the PM decisions are made?

I am talking with our PM to see how we can handle that.
What release are we targeting this?

@semd This is planned only for the next minor release, so 9.1.0

@tsullivan tsullivan removed ci:project-deploy-elasticsearch Create an Elasticsearch Serverless project ci:project-deploy-observability Create an Observability project labels Feb 20, 2025
@semd
Copy link
Contributor

semd commented Feb 21, 2025

If we need to add changes to the Security Solution navigation tree in this PR, let's make sure we do that. I will probably need your guidance on where to add the code. Would you be able to send me a PR to this branch after the PM decisions are made?

@tsullivan Yes, I can do that. I think we should keep this PR on hold until then. I'll keep you updated.

kdelemme
kdelemme approved these changes Feb 21, 2025
View reviewed changes
Copy link
Contributor

@kdelemme kdelemme left a comment

Choose a reason for hiding this comment

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

obs-ux-management code change LGTM

@botelastic botelastic bot added the ci:project-deploy-observability Create an Observability project label Feb 21, 2025
@github-actions GitHub Actions
Copy link
Contributor

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • run docs-build : Re-trigger the docs validation. (use unformatted text in the comment!)

@semd semd mentioned this pull request Mar 3, 2025
Open
@elasticmachine
Copy link
Contributor

elasticmachine commented Mar 5, 2025 •
edited
Loading

💛 Build succeeded, but was flaky

  • Buildkite Build
  • Commit: 1e4106f
  • Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-210893-1e4106f5576a

Failed CI Steps

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
navigation 20.0KB 18.6KB -1.4KB
serverless 20.8KB 19.4KB -1.4KB
total -2.8KB

History

cc @tsullivan

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@semd semd semd left review comments

@kdelemme kdelemme kdelemme approved these changes

@dmlemeshko dmlemeshko dmlemeshko approved these changes

@kowalczyk-krzysztof kowalczyk-krzysztof kowalczyk-krzysztof approved these changes

At least 1 approving review is required to merge this pull request.

Assignees

@tsullivan tsullivan

Labels
backport:skip This commit does not require backporting blocked ci:project-deploy-observability Create an Observability project release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@tsullivan @elasticmachine @semd @kdelemme @dmlemeshko @kowalczyk-krzysztof