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

[Serverless nav] Accordion auto-expand state + polish work #169651

Merged
merged 16 commits into from
Oct 26, 2023
Merged

[Serverless nav] Accordion auto-expand state + polish work #169651

merged 16 commits into from
Oct 26, 2023

Conversation

sebelga
Copy link
Contributor

@sebelga sebelga commented Oct 24, 2023 •
edited
Loading

In this PR I fixed the auto-expand behaviour for the nav accordions if one of their item is "active" (matches the URL location). I've updated the Observability nav to correctly render the accordion with the chevron.

How to test

Launch the serverless Observability

yarn serverless-oblt

Other fixes

Show the collapsed nav with icon

Screenshot 2023-10-25 at 16 14 26

Correctly set the active state of root nav items

Screenshot 2023-10-25 at 16 14 46

Set the active state on the accordion title if it is collapsed (and any of its children is active)

Screenshot 2023-10-25 at 16 14 57

Known issue

There is a UI "glitch" when the last accordion is expanded/collapsed. I looked into it and it seems related to the block-size inline style that gets added by EUI. cc @cee-chen

It is being fixed in elastic/eui#7317

accordion-glitch

Fixes #167328
Fixes #167330

@sebelga sebelga force-pushed the serverless-chrome/accordion-collapsed-state branch 2 times, most recently from 9c07de9 to 08c3daf Compare October 25, 2023 10:04
This was referenced Oct 25, 2023
Closed
Closed
Closed
@sebelga sebelga changed the title [Serverless] Accordion auto-expand state [Serverless nav] Accordion auto-expand state + polish work Oct 25, 2023
@sebelga sebelga force-pushed the serverless-chrome/accordion-collapsed-state branch from 890612c to a7607f9 Compare October 25, 2023 13:47
@sebelga sebelga self-assigned this Oct 25, 2023
@sebelga sebelga added Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) Project:Serverless Work as part of the Serverless project for its initial release Project:Serverless MVP labels Oct 25, 2023
@sebelga sebelga marked this pull request as ready for review October 25, 2023 14:20
@sebelga sebelga requested a review from a team as a code owner October 25, 2023 14:20
@sebelga sebelga requested a review from a team October 25, 2023 14:20
@sebelga sebelga requested a review from a team as a code owner October 25, 2023 14:20
@elasticmachine
Copy link
Contributor

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

@sebelga sebelga requested a review from Dosant October 25, 2023 14:20
@sebelga sebelga added the release_note:skip Skip the PR/issue when compiling release notes label Oct 25, 2023
@sebelga sebelga requested a review from a team as a code owner October 25, 2023 19:52
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Security Solution Cypress Tests #1 / Enrichment Custom query rule from new risk scores Should has enrichment fields from legacy risk Should has enrichment fields from legacy risk

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 48 47 -1

Async chunks

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

id before after diff
securitySolutionServerless 334.3KB 336.0KB +1.8KB

Page load bundle

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

id before after diff
core 371.6KB 371.6KB -11.0B
serverlessObservability 60.8KB 62.5KB +1.6KB
serverlessSearch 47.2KB 49.0KB +1.8KB
total +3.4KB
Unknown metric groups

API count

id before after diff
@kbn/shared-ux-chrome-navigation 60 58 -2

History

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

cc @sebelga

sphilipse
sphilipse approved these changes Oct 26, 2023
View reviewed changes
Copy link
Member

@sphilipse sphilipse left a comment

Choose a reason for hiding this comment

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

search changes LGTM!

@sebelga sebelga requested a review from semd October 26, 2023 08:15
semd
semd approved these changes Oct 26, 2023
View reviewed changes
Copy link
Contributor

@semd semd left a comment

Choose a reason for hiding this comment

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

Security changes LGTM!

Dosant
Dosant approved these changes Oct 26, 2023
View reviewed changes
Copy link
Contributor

@Dosant Dosant left a comment

Choose a reason for hiding this comment

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

Great work.

Since the nav is collapsed now instead of hidden, I think we can close this, correct? #169355

packages/shared-ux/chrome/navigation/src/ui/components/navigation_section_ui.tsx

import React, { FC, useCallback, useEffect, useMemo, useState } from 'react';
import classNames from 'classnames';
import { css } from '@emotion/css';
Copy link
Contributor

@Dosant Dosant Oct 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

nit: any reason to use /css instead of /react? https://emotion.sh/docs/best-practices#if-using-react-prefer-emotionreact-or-emotionstyled-over-emotioncss

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I didn't even realized 😄 I guess both work

@Dosant Dosant mentioned this pull request Oct 26, 2023
Closed
yngrdyn
yngrdyn approved these changes Oct 26, 2023
View reviewed changes
Copy link
Contributor

@yngrdyn yngrdyn left a comment

Choose a reason for hiding this comment

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

APM Changes LGTM

@sebelga
Copy link
Contributor Author

sebelga commented Oct 26, 2023

Since the nav is collapsed now instead of hidden, I think we can close this, correct?

No it is still relevant. I am not sure I would bother much with that issue to be honest, I don't think many users try going mobile on their desktop and then back to desktop view.

@sebelga sebelga merged commit b759b8f into elastic:main Oct 26, 2023
@sebelga sebelga deleted the serverless-chrome/accordion-collapsed-state branch October 26, 2023 09:29
@kibanamachine kibanamachine added v8.12.0 backport:skip This commit does not require backporting labels Oct 26, 2023
@sebelga sebelga mentioned this pull request Oct 31, 2023
Closed
@sebelga sebelga mentioned this pull request Nov 7, 2023
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yngrdyn yngrdyn yngrdyn approved these changes

@Dosant Dosant Dosant approved these changes

@semd semd semd approved these changes

@sphilipse sphilipse sphilipse approved these changes

Assignees

@sebelga sebelga

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:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) v8.12.0
Projects
None yet
Milestone
No milestone
8 participants
@sebelga @elasticmachine @kibana-ci @yngrdyn @Dosant @semd @sphilipse @kibanamachine