Refactor workspace context menu register #207

ruanyl · 2023-09-28T07:49:09Z

Description

Issues Resolved

Screenshot

Testing the changes

Check List

Signed-off-by: Yulong Ruan <ruanyl@amazon.com>

codecov-commenter · 2023-09-28T08:07:07Z

Codecov Report

Merging #207 (21f8e03) into workspace (29ccc20) will increase coverage by 30.37%.
Report is 5 commits behind head on workspace.
The diff coverage is 58.33%.

@@              Coverage Diff               @@
##           workspace     #207       +/-   ##
==============================================
+ Coverage      30.42%   60.79%   +30.37%     
==============================================
  Files           2199     2991      +792     
  Lines          45154    58895    +13741     
  Branches        7023     9671     +2648     
==============================================
+ Hits           13737    35804    +22067     
+ Misses         30810    20991     -9819     
- Partials         607     2100     +1493

Flag	Coverage Δ
Linux_1	`30.42% <8.33%> (+<0.01%)`	⬆️
Linux_2	`55.63% <58.33%> (?)`
Linux_3	`42.76% <8.33%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
src/core/public/chrome/chrome_service.mock.ts	`100.00% <100.00%> (+18.75%)`	⬆️
src/core/public/chrome/ui/header/header.tsx	`75.00% <ø> (+75.00%)`	⬆️
src/core/public/core_system.ts	`92.04% <100.00%> (+92.04%)`	⬆️
src/core/public/plugins/plugin_context.ts	`44.44% <ø> (+44.44%)`	⬆️
...c/core/public/workspace/workspaces_service.mock.ts	`100.00% <ø> (ø)`
src/core/public/workspace/workspaces_service.ts	`90.00% <100.00%> (+90.00%)`	⬆️
...c/core/public/chrome/ui/header/collapsible_nav.tsx	`96.36% <50.00%> (+96.36%)`	⬆️
src/core/public/chrome/chrome_service.tsx	`86.66% <20.00%> (+85.23%)`	⬆️

... and 1608 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Signed-off-by: Yulong Ruan <ruanyl@amazon.com>

SuZhou-Joe · 2023-10-10T05:22:22Z

src/core/public/chrome/chrome_service.tsx

@@ -181,6 +196,17 @@ export class ChromeService {
      localStorage.setItem(IS_LOCKED_KEY, `${isLocked}`);
    };

+    const collapsibleNavHeaderRender = this.collapsibleNavHeaderRender
+      ? () =>
+          this.collapsibleNavHeaderRender


Duplicate judgement on this.collapsibleNavHeaderRender, maybe we only need one?

Good point, updated now

Signed-off-by: Yulong Ruan <ruanyl@amazon.com>

yuye-aws · 2023-10-10T05:37:04Z

src/core/public/chrome/ui/header/collapsible_nav.tsx

@@ -197,7 +196,7 @@ export function CollapsibleNav({
      outsideClickCloses={false}
    >
      <EuiFlexItem className="eui-yScroll">
-        <CollapsibleNavHeader workspaces={workspaces} />
+        {collapsibleNavHeaderRender ? collapsibleNavHeaderRender() : <CollapsibleNavHeader />}


By your PR, CollapsibleNavHeader would simply be a react component, I would suggest moving CollapsibleNavHeader implementation to collapsible_nav.tsx and the header file is no longer needed

Good point, I will update

yuye-aws · 2023-10-10T05:44:32Z

src/plugins/workspace/public/components/workspace_menu/workspace_menu.tsx

@@ -33,7 +33,7 @@ import { formatUrlWithWorkspaceId } from '../../utils';
 interface Props {
  getUrlForApp: ApplicationStart['getUrlForApp'];
  basePath: HttpSetup['basePath'];
-  observables: WorkspaceObservables;
+  workspaces: WorkspaceObservables;


Should it be WorkspacesStart?

yuye-aws · 2023-10-10T05:45:27Z

src/core/public/workspace/workspaces_service.ts

-export interface WorkspacesStart extends WorkspaceObservables {
-  renderWorkspaceMenu: () => JSX.Element | null;
-}
+export type WorkspacesSetup = WorkspaceObservables;


Do we still need to export type WorkspaceObservables?

If WorkspaceObservables is no longer needed, we can reset the code as the following:

export interface WorkspaceStart { currentWorkspaceId$: BehaviorSubject<string>; currentWorkspace$: BehaviorSubject<WorkspaceAttribute | null>; workspaceList$: BehaviorSubject<WorkspaceAttribute[]>; workspaceEnabled$: BehaviorSubject<boolean>; } export type WorkspaceSetup = WorkspaceStart;

nice catch, this is not needed now.

I will keep WorkspaceObservables for now, should not be an issue, I think it will be needed in the future

Signed-off-by: Yulong Ruan <ruanyl@amazon.com>

yuye-aws · 2023-10-10T06:14:34Z

src/core/public/chrome/ui/header/collapsible_nav.tsx

@@ -132,11 +134,11 @@ interface Props {
  navigateToUrl: InternalApplicationStart['navigateToUrl'];
  customNavLink$: Rx.Observable<ChromeNavLink | undefined>;
  logos: Logos;
-  workspaces: WorkspacesStart;


Also need to remove this prop defined in collapsible_nav.test.tsx

yuye-aws · 2023-10-10T06:19:12Z

src/core/public/chrome/ui/header/__snapshots__/collapsible_nav.test.tsx.snap

@@ -407,89 +407,13 @@ exports[`CollapsibleNav renders links grouped by category 1`] = `
        "observers": Array [],
        "thrownError": null,
      },
-      "renderWorkspaceMenu": [MockFunction],
      "workspaceEnabled$": BehaviorSubject {


The snapshot file should be updated along with test file

All four snapshot files should be updated.

Signed-off-by: Yulong Ruan <ruanyl@amazon.com>

yuye-aws · 2023-10-10T07:21:20Z

src/core/public/chrome/ui/header/header.tsx

@@ -94,7 +94,6 @@ export interface HeaderProps {
  branding: ChromeBranding;
  logos: Logos;
  survey: string | undefined;
-  workspaces: WorkspacesStart;


Need to remove corresponding prop in header.test.tsx

Signed-off-by: Yulong Ruan <ruanyl@amazon.com>

src/core/public/chrome/chrome_service.tsx

yuye-aws · 2023-10-10T07:49:12Z

...ublic/application/components/dashboard_listing/__snapshots__/dashboard_listing.test.tsx.snap

@@ -5789,7 +5785,6 @@ exports[`dashboard listing renders warning when listingLimit is exceeded 1`] = `
                "observers": Array [],
                "thrownError": null,
              },
-              "renderWorkspaceMenu": [MockFunction],
              "workspaceEnabled$": BehaviorSubject {


Need update?

This PR doesn't include change of workspaceEnabled$.

yuye-aws · 2023-10-10T07:49:20Z

...ublic/application/components/dashboard_top_nav/__snapshots__/dashboard_top_nav.test.tsx.snap

@@ -892,7 +892,6 @@ exports[`Dashboard top nav render in embed mode 1`] = `
                "observers": Array [],
                "thrownError": null,
              },
-              "renderWorkspaceMenu": [MockFunction],
              "workspaceEnabled$": BehaviorSubject {


Need update?

Hailong-am · 2023-10-10T09:20:52Z

src/core/public/chrome/chrome_service.tsx

+      registerCollapsibleNavHeader: (render: CollapsibleNavHeaderRender) => {
+        this.collapsibleNavHeaderRender = render;
+      },
+    };


is it ok to set collapsibleNavHeaderRender multiple times to override previous one, or we should add a check here to let it only set it once?

You mean if different plugins called registerCollapsibleNavHeader with different render, previous one will be override?
Actually I have the same concern, but it should be fine as this will only be used by workspace for now. If in the future, this is called by different plugins, we may add the check or register the render to an array, I don't know which is expected, so just leave it like this for now.

ruanyl added 5 commits September 26, 2023 17:55

remove unnecessary workspace menu register

6cecd9a

Signed-off-by: Yulong Ruan <ruanyl@amazon.com>

expose interface from chrome service to allow customize left nav header

218138b

Signed-off-by: Yulong Ruan <ruanyl@amazon.com>

update tests + cleanup

69efb0d

Signed-off-by: Yulong Ruan <ruanyl@amazon.com>

fix type errors

9fc06f7

Signed-off-by: Yulong Ruan <ruanyl@amazon.com>

revert WorkspaceMenu component

37234f3

Signed-off-by: Yulong Ruan <ruanyl@amazon.com>

ruanyl requested review from wanglam, SuZhou-Joe, yuye-aws, raintygao and Hailong-am September 28, 2023 07:49

github-actions bot added the seasoned-contributor label Sep 28, 2023

remove unnecessary code

17832fb

Signed-off-by: Yulong Ruan <ruanyl@amazon.com>

SuZhou-Joe reviewed Oct 10, 2023

View reviewed changes

cleanup unnecessary code

a315e21

Signed-off-by: Yulong Ruan <ruanyl@amazon.com>

yuye-aws reviewed Oct 10, 2023

View reviewed changes

update per pr comment

9f0c054

Signed-off-by: Yulong Ruan <ruanyl@amazon.com>

yuye-aws reviewed Oct 10, 2023

View reviewed changes

fix collapsibleNavHeaderRender

6ed1af8

Signed-off-by: Yulong Ruan <ruanyl@amazon.com>

yuye-aws reviewed Oct 10, 2023

View reviewed changes

fix tests

21f8e03

Signed-off-by: Yulong Ruan <ruanyl@amazon.com>

wanglam reviewed Oct 10, 2023

View reviewed changes

src/core/public/chrome/chrome_service.tsx Show resolved Hide resolved

yuye-aws reviewed Oct 10, 2023

View reviewed changes

wanglam approved these changes Oct 10, 2023

View reviewed changes

yuye-aws approved these changes Oct 10, 2023

View reviewed changes

raintygao approved these changes Oct 10, 2023

View reviewed changes

Hailong-am reviewed Oct 10, 2023

View reviewed changes

ruanyl merged commit b935e36 into workspace Oct 10, 2023
41 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor workspace context menu register #207

Refactor workspace context menu register #207

ruanyl commented Sep 28, 2023

codecov-commenter commented Sep 28, 2023 •

edited

Loading

SuZhou-Joe Oct 10, 2023

ruanyl Oct 10, 2023

yuye-aws Oct 10, 2023 •

edited

Loading

ruanyl Oct 10, 2023

yuye-aws Oct 10, 2023

yuye-aws Oct 10, 2023

yuye-aws Oct 10, 2023

ruanyl Oct 10, 2023 •

edited

Loading

ruanyl Oct 10, 2023

yuye-aws Oct 10, 2023

yuye-aws Oct 10, 2023

yuye-aws Oct 10, 2023

yuye-aws Oct 10, 2023

yuye-aws Oct 10, 2023 •

edited

Loading

ruanyl Oct 10, 2023

yuye-aws Oct 10, 2023 •

edited

Loading

Hailong-am Oct 10, 2023

ruanyl Oct 10, 2023

Refactor workspace context menu register #207

Refactor workspace context menu register #207

Conversation

ruanyl commented Sep 28, 2023

Description

Issues Resolved

Screenshot

Testing the changes

Check List

codecov-commenter commented Sep 28, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuye-aws Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruanyl Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuye-aws Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuye-aws Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 28, 2023 •

edited

Loading

yuye-aws Oct 10, 2023 •

edited

Loading

ruanyl Oct 10, 2023 •

edited

Loading

yuye-aws Oct 10, 2023 •

edited

Loading

yuye-aws Oct 10, 2023 •

edited

Loading