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

[SharedUx] Chrome/Navigation package #152510

Merged
merged 46 commits into from
Apr 28, 2023

Conversation

tsullivan
Copy link
Member

@tsullivan tsullivan commented Mar 1, 2023

The API for the link definition is subject to change. This PR provides linking functionality and structure that will give solution devs a starting point for side nav in their projects. The API uses simple hrefs for now, because it's the easiest thing to start with. Planning forward, we are thinking of a different navigation model that separates nav structure from presentation of each nav item - this will allow reuse of the structure without reusing the presentation.

Isolated dependencies

In order for this component to be usable in main, a bit further work is currently required in the ChromeStart service and the Serverless plugin. These links are examples of a usable implementation that link to a POC branch:

Summary

Introduces a component to host the side navigation in Kibana. Solution teams can insert their own content, and have other small options to customize the presentation: see the storybook demos for more.

Closes #154479
Closes #154484
Closes #154485
Closes #154489
Closes #154481
Closes #154480
Closes #154486
Closes #154487

image

Developer documentation

See the Storybook demos:

  • run: yarn storybook shared_ux
  • Find the Chrome > Navigation section in the Storybook app

Checklist

Delete any items that are not applicable to this PR.

  • Home icon links to Project's "home" - or the customer user setting
  • Home icon shows loading indicator
  • All the Platform links navigate to the correct place
  • Platform links are not shown if the underlying plugin is disabled
  • Nav items define their links using href only
  • All href links work
  • Nav menu item to link to Cloud deployment
  • Unit or functional tests were updated or added to match the most common scenarios
  • Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
  • Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
  • This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
  • This was checked for cross-browser compatibility

@tsullivan tsullivan force-pushed the shared-ux/chrome/navigation branch 5 times, most recently from 2d2c3ad to dbb0f8b Compare March 2, 2023 02:00
@clintandrewhall
Copy link
Contributor

Love this...! A great start. 🎉 🚀

@tsullivan tsullivan force-pushed the shared-ux/chrome/navigation branch from 182ef95 to bd50859 Compare March 7, 2023 20:48
@tsullivan tsullivan self-assigned this Mar 7, 2023
@tsullivan tsullivan force-pushed the shared-ux/chrome/navigation branch 3 times, most recently from 203a178 to 2a34e45 Compare March 8, 2023 05:11
@tsullivan tsullivan added the Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) label Mar 8, 2023
@tsullivan tsullivan force-pushed the shared-ux/chrome/navigation branch 2 times, most recently from 2f33215 to 3ceafe5 Compare March 23, 2023 00:27
@tsullivan tsullivan force-pushed the shared-ux/chrome/navigation branch 2 times, most recently from d90ee02 to 417252a Compare April 6, 2023 00:17
@tsullivan tsullivan requested review from dgieselaar and semd April 11, 2023 16:18
@tsullivan tsullivan force-pushed the shared-ux/chrome/navigation branch from 1e5aa3d to 6f409e7 Compare April 12, 2023 21:30
@tsullivan tsullivan force-pushed the shared-ux/chrome/navigation branch from 22c4cf5 to ead32d6 Compare April 20, 2023 22:28
@dgieselaar
Copy link
Member

@tsullivan what kind of feedback are you looking for on this PR? I see that the API is subject to change, so not sure how deep we should go into that. I see a couple of visual bugs but as this is a first iteration those are probably not critical either?

@tsullivan
Copy link
Member Author

@tsullivan what kind of feedback are you looking for on this PR? I see that the API is subject to change, so not sure how deep we should go into that. I see a couple of visual bugs but as this is a first iteration those are probably not critical either?

Hi @dgieselaar! Yes, the API for the link definition is subject to change. This PR provides linking functionality and structure that will give you a starting point for side nav in your project. e're just using hrefs in the API for now, because it's the easiest thing to start with.

Meanwhile, my team will ask for your feedback and try address it for our future iterations. We are thinking of a different navigation model that separates nav structure from presentation of each nav item - this will allow reuse of the structure without reusing the presentation. We realized we need that approach to give solution teams the option to have a highly customized side nav, and allow our breadcrumb component to "just work."

Comment on lines 35 to 38
getActiveNavItemId$: () => Observable<string | undefined>;
getProjectNavIsOpen$: () => Observable<boolean>;
recentlyAccessed: { get$: () => Observable<RecentItem[]> };
registerNavItemClick: (id: string) => void;
Copy link
Member Author

@tsullivan tsullivan Apr 27, 2023

Choose a reason for hiding this comment

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

This includes extensions that are currently not provided by the ChromeStart service. These extensions were used to work out active link highlighting, but we are planning to change the API so that active link highlighting and breadcrumbs can work based on something more structured in the link definition. I will remove this to make a clean PR that will work with minimal changes needed in ChromeStart.

For determining if the side nav is open or collapsed, there will be a local storage setting that consumers can use, or we can make a service for it in the chrome package.

Copy link
Member Author

Choose a reason for hiding this comment

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

resolved in d1cd551

Copy link
Contributor

@ryankeairns ryankeairns left a comment

Choose a reason for hiding this comment

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

We'll make some UI polish passes post-merge / in the coming weeks.

@kibana-ci
Copy link
Collaborator

💚 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 - 11 +11

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 - 3 +3
Unknown metric groups

API count

id before after diff
@kbn/shared-ux-chrome-navigation - 21 +21

ESLint disabled line counts

id before after diff
enterpriseSearch 17 19 +2
securitySolution 399 402 +3
total +5

Total ESLint disabled count

id before after diff
enterpriseSearch 18 20 +2
securitySolution 479 482 +3
total +5

History

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

cc @tsullivan

@tsullivan
Copy link
Member Author

Since the first reviews of this, there have only been removals of ChromeStart integrations that are currently going through redesign.

basePath,
loadingCount,
navigateToUrl,
navIsOpen: true,
Copy link
Member Author

Choose a reason for hiding this comment

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

In the next few iterations, we'll refine how the nav is aware of it's open/closed status, and it will use a local storage setting.

@tsullivan tsullivan merged commit 604a02f into elastic:main Apr 28, 2023
@tsullivan tsullivan deleted the shared-ux/chrome/navigation branch April 28, 2023 00:13
@kibanamachine kibanamachine added v8.9.0 backport:skip This commit does not require backporting labels Apr 28, 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 ci:build-storybooks 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