-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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 V2] Remove "v1" implementation #158919
Conversation
Pinging @elastic/appex-sharedux (Team:SharedUX) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few small comments about organization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few small comments about organization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We must include the "recently accessed" slot in the solution navs
x-pack/plugins/serverless_observability/public/components/side_navigation/index.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Co-authored-by: Tim Sullivan <tsullivan@users.noreply.github.com>
Thanks for the review @tsullivan ! I applied your changes, can you have another look? Cheers 👍 |
Co-authored-by: Tim Sullivan <tsullivan@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
limits.yml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Observability serverless changes LGTM.
I still see the bug #158376. Will be handled in a separate PR?
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like #158376 requires a minimal fix, so we should do it in this PR
import { ChromeProjectNavigationNodeEnhanced } from '../types'; | ||
|
||
const navigationNodeToEuiItem = ( | ||
item: ChromeProjectNavigationNodeEnhanced, | ||
{ navigateToUrl, basePath }: { navigateToUrl: NavigateToUrlFn; basePath: BasePathService } | ||
): EuiSideNavItemType<unknown> => { | ||
const href = item.deepLink?.href; | ||
const href = item.deepLink?.href ?? item.href; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we should prepend the basePath to href in a single place, to solve #158376 and not repeat the code.
Maybe something like:
const rootHref = item.deepLink?.href ?? item.href;
const href = rootHref ? basePath.prepend(rootHref) : undefined;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you don't mind I will address this in a following PR. As I mentioned earlier the href are going away in the following PR so I don't think we should fix it.
Thanks for the review @kpatticha
I am working on #158658 which will remove support for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm approving this for @sebelga to be able to merge this in his working time. I think it's important to address the fix for opening links in a new tab, since solutions teams need that for testing. I took a few minutes with it, and can propose this patch:
commit bee6dabd4ae9ed73ce79908cb311ad946c837e4a
Author: Timothy Sullivan <tsullivan@elastic.co>
Date: Mon Jun 5 10:46:08 2023 -0700
fix open-in-new-tab
diff --git a/packages/shared-ux/chrome/navigation/src/ui/components/navigation_section_ui.tsx b/packages/shared-ux/chrome/navigation/src/ui/components/navigation_section_ui.tsx
index a592e6cc539..96913040494 100644
--- a/packages/shared-ux/chrome/navigation/src/ui/components/navigation_section_ui.tsx
+++ b/packages/shared-ux/chrome/navigation/src/ui/components/navigation_section_ui.tsx
@@ -23,19 +23,22 @@ const navigationNodeToEuiItem = (
item: ChromeProjectNavigationNodeEnhanced,
{ navigateToUrl, basePath }: { navigateToUrl: NavigateToUrlFn; basePath: BasePathService }
): EuiSideNavItemType<unknown> => {
- const href = item.deepLink?.href ?? item.href;
+ let href = item.deepLink?.href ?? item.href;
const id = item.path ? item.path.join('.') : item.id;
+ let onClick: undefined | ((event: React.MouseEvent) => void);
+ if (href) {
+ const fullHref = (href = basePath.prepend(href));
+ onClick = (event: React.MouseEvent) => {
+ event.preventDefault();
+ navigateToUrl(fullHref);
+ };
+ }
+
return {
id,
name: item.title,
- onClick:
- href !== undefined
- ? (event: React.MouseEvent) => {
- event.preventDefault();
- navigateToUrl(basePath.prepend(href!));
- }
- : undefined,
+ onClick,
href,
renderItem: item.renderItem,
items: item.children?.map((_item) =>
This PR replaces the "v1" version of the shared ux chrome navigation package with the new "v2" components introduced in #158297.
Changes
href
along withlink
(deeplink id). This will be removed in a following PRserverless_search
andserverless_observability
side navigation.Notes for reviewer
href
prop to be passed (and fixing the tests) there hasn't been an changed in the implementation from [SharedUxChromeNavigation V2] Add EUI component to building blocks #158297. Meaning that all the "changes" in thenavigation.stories.ts
files are simply the result of overriding version 1 with version 2.yarn serverless oblt
|yarn serverless es
and make sure the side navigation works as expected.Fixes #157707