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

[SharedUxChromeNavigation] Detect active nav route(s) #159906

Merged
merged 40 commits into from
Jun 22, 2023

Conversation

sebelga
Copy link
Contributor

@sebelga sebelga commented Jun 19, 2023

This PR add the active navigation detection in the side navigation.

getActiveNavigationNodes$()

The serverless plugin exposes a new getActiveNavigationNodes$() which returns the active nodes in the navigation tree based on the current browser location. The active node is the one with the longest url match.

Location.pathname = "/foo/bar/baz"
node1.url = "/foo"
node2.url = "/foo/bar" --> match

The result is a jagged array of branches of active nodes.

[
  [
    { id: 'root', path: ['root'] },
    { id: 'group1', title:'Group 1' path: ['root', 'group1'] },
    { id: 'item1', title:'Item 1', path: ['root', 'group1', 'item1'] }
  ],
  // If there are 2 branches that matches, 
  [
    { id: 'root', path: ['root'] },
    { id: 'item1', title:'Item 1', path: ['root', 'item1'] }
  ],
]

getIsActive()

It is possible to override the default matching mechanism and provide a handler to set the active state of a node. This handler is called whenever the location changes.

{
  id: 'alerts-cases-slos',
  children: [
    {
      link: 'observability-overview:alerts',
      // handler to set the active state
      getIsActive: (location) => {
        return location.pathname.startsWith('/alerts');
      },
    },
    ...
  ],
},

// Or using components
<Navigation.Group id="group:settings" title="Settings">
  <Navigation.Item
    id="logs"
    title="Logs"
    getIsActive={(location) => {
      return location.pathname.startsWith('/logs');
    }}
  />
  ...
</Navigation.Group>

Note

There is an issue to change the style of the active state. This PR does not solve it as it should be done in EUI. We use the isSelected state declared on the items of the <EuiSideNav /> component. When this component is updated the new style will be applied here.

Screenshot

Screenshot 2023-06-20 at 08 56 01

Fixes #156664

@sebelga sebelga force-pushed the chrome-nav/detect-active-nav-route branch from 1e73505 to af27ff8 Compare June 20, 2023 10:00
@sebelga sebelga self-assigned this Jun 20, 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 release_note:skip Skip the PR/issue when compiling release notes labels Jun 20, 2023
@sebelga sebelga marked this pull request as ready for review June 20, 2023 10:25
@sebelga sebelga requested review from a team as code owners June 20, 2023 10:25
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.

Nice! Have some questions/suggestions. In the meantime, I'll start working on breadcrumbs and see if I have any more questions

@@ -107,6 +107,15 @@ export const NavigationSectionUI: FC<Props> = ({

const groupHasLink = Boolean(navNode.deepLink) || Boolean(navNode.href);

useEffect(() => {
// We only want to set the collapsed state during the "mounting" phase (500ms).
Copy link
Contributor

Choose a reason for hiding this comment

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

is it possible to track if the section was manually opened and in that case not auto-collapse it or something like this?
Or maybe possible to rely on the initial render?

Copy link
Contributor

Choose a reason for hiding this comment

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

What is "the "mounting" phase" in this sense? Does this mean that initially "isActive" is incorrect? Why so? Can we make it correct on the first render?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think related #159906 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this mean that initially "isActive" is incorrect? Why so?

I am not sure it would be possible or would require quite a complex logic.

The state order is:

  • isActive: false
  • item register themselves in their parents
  • the root Navigation component register the tree in the ChromeService
  • the ChromeService updates the activeNodes based on the navigation tree + the current location
  • the active nodes are returned to the Navigation
  • each node checks if it is active and updates the isActive state

That's why all node initially mount as isActive: false and only after they are mounted the activeNodes array is constructed.

is it possible to track if the section was manually opened and in that case not auto-collapse it or something like this?

It is not only when it was manually opened, also if it was opened automatically on page load because one of the children is active.

You can disable the if statement around setIsCollapsed(!isActive); and see for yourself why it is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basically we don't want to close automatically a group that has been opened (manually or automatically).

Copy link
Member

@jbudz jbudz left a comment

Choose a reason for hiding this comment

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

limits.yml

@sebelga
Copy link
Contributor Author

sebelga commented Jun 21, 2023

Thanks for the review @Dosant ! I addressed all of your feedback, can you have another look? Cheers!

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
core 429 431 +2
serverlessObservability 35 37 +2
serverlessSearch 81 83 +2
total +6

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/core-chrome-browser 58 61 +3
@kbn/shared-ux-chrome-navigation 41 36 -5
serverless 15 16 +1
total -1

Async chunks

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

id before after diff
core 145.5KB 145.5KB +6.0B

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
@kbn/shared-ux-chrome-navigation 5 4 -1

Page load bundle

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

id before after diff
core 353.9KB 357.1KB +3.2KB
serverless 5.3KB 5.3KB +75.0B
serverlessObservability 16.5KB 17.9KB +1.4KB
serverlessSearch 19.8KB 21.2KB +1.4KB
total +6.1KB
Unknown metric groups

API count

id before after diff
@kbn/core-chrome-browser 149 155 +6
@kbn/shared-ux-chrome-navigation 62 45 -17
serverless 16 17 +1
total -10

ESLint disabled line counts

id before after diff
@kbn/core-chrome-browser 1 0 -1
enterpriseSearch 13 15 +2
securitySolution 416 420 +4
total +5

Total ESLint disabled count

id before after diff
@kbn/core-chrome-browser 1 0 -1
enterpriseSearch 14 16 +2
securitySolution 497 501 +4
total +5

History

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

cc @sebelga

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.

I tested while working on breadcrumbs 👍

@sebelga sebelga merged commit 39c738b into elastic:main Jun 22, 2023
@sebelga sebelga deleted the chrome-nav/detect-active-nav-route branch June 22, 2023 10:30
@kibanamachine kibanamachine added v8.9.0 backport:skip This commit does not require backporting labels Jun 22, 2023
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 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.9.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Navigation] Detect active nav route(s) from history state and pass it to left nav prop
6 participants