Add workspace create page #284

wanglam · 2024-03-06T08:23:40Z

Description

Add workspace create page

Issues Resolved

Screenshot

Testing the changes

Check List

All tests pass
- yarn test:jest
- yarn test:jest_integration
New functionality includes testing.
New functionality has been documented.
Update CHANGELOG.md
Commits are signed per the DCO using --signoff

codecov-commenter · 2024-03-06T08:48:26Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.91%. Comparing base (0cef35d) to head (a85666c).

Additional details and impacted files

@@                   Coverage Diff                   @@
##           workspace-pr-integr     #284      +/-   ##
=======================================================
- Coverage                66.92%   66.91%   -0.01%     
=======================================================
  Files                     2598     2598              
  Lines                    49149    49149              
  Branches                  8890     8890              
=======================================================
- Hits                     32891    32889       -2     
- Misses                   14090    14091       +1     
- Partials                  2168     2169       +1

Flag	Coverage Δ
_2	`55.38% <ø> (ø)`
_3	`43.32% <ø> (-0.01%)`	⬇️
_4	`35.15% <ø> (ø)`

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

SuZhou-Joe

Left some comment, though I know some of which are others' work ^^.

SuZhou-Joe · 2024-03-11T10:30:28Z

src/plugins/workspace/public/components/workspace_creator/workspace_bottom_bar.tsx

+
+interface WorkspaceBottomBarProps {
+  formId: string;
+  opType?: string;


Can we add an enum for opType?

SuZhou-Joe · 2024-03-11T10:32:24Z

src/plugins/workspace/public/components/workspace_creator/workspace_bottom_bar.tsx

+              {opType === WORKSPACE_OP_TYPE_UPDATE ? (
+                <EuiText textAlign="left">
+                  {i18n.translate('workspace.form.bottomBar.unsavedChanges', {
+                    defaultMessage: `${numberOfUnSavedChanges} Unsaved change(s)`,
+                  })}
+                </EuiText>
+              ) : (
+                <EuiText textAlign="left">
+                  {i18n.translate('workspace.form.bottomBar.errors', {
+                    defaultMessage: `${numberOfErrors} Error(s)`,
+                  })}
+                </EuiText>
+              )}


It seems only the default message can replace the ${numberOfErrors}, translate function should support placeholder in some way.

SuZhou-Joe · 2024-03-11T10:38:21Z

src/plugins/workspace/public/components/workspace_creator/workspace_creator.test.tsx

+        getUrlForApp: jest.fn(),
+        applications$: new BehaviorSubject<Map<string, PublicAppInfo>>(PublicAPPInfoMap as any),
+      },
+      http: {


Any reason we add http entry here? I think mockCoreStart should have mocked all the functions under http

You're right, can be removed.

SuZhou-Joe · 2024-03-11T10:42:19Z

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

+          }),
+        });
+        if (application && http) {
+          window.location.href = formatUrlWithWorkspaceId(


Do we need to set a timeout before jumping to the workspace? Or user may not have enough time to see the notifications toast.

Is the workspace id stored in the hash part? If this part of refactor was been finalized, we can use application.navigateToUrl instead. Then the page won't be reload. The toasts will be kept. Then we don't need to add delay.

I see, we will use the path option(current behavior in workspace branch) to store the workspace id. So I guess we may need a timeout here.

Will add an one second timeout here.

SuZhou-Joe · 2024-03-11T10:43:09Z

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

+          paddingSize="none"
+          color="subdued"
+          hasShadow={false}
+          style={{ width: '100%', maxWidth: 1000 }}


What is the maxWidth: 1000 used for?

This maxWidth: 1000 is for align with the above page header title.

It will strength full width if not set.

I see, can we add a comment here? For someone without the context like me may be confused on this style property.

SuZhou-Joe · 2024-03-11T10:47:40Z

src/plugins/workspace/public/components/workspace_creator/workspace_form.tsx

This component is too large(714 lines) and collapsed by default when review, can we split this file into several smaller files(types, components, hooks, utils..) to improve readability.

SuZhou-Joe · 2024-03-11T10:50:29Z

...plugins/workspace/public/components/workspace_creator/workspace_permission_setting_panel.tsx

Component is so large that collapsed by default, seems many codes are used for constants, can we separate to multiple smaller files?

Signed-off-by: Lin Wang <wonglam@amazon.com>

Hailong-am · 2024-03-12T02:36:45Z

src/core/public/application/types.ts

+    [key: string]: {
+      type: 'required' | 'optional';
+    };
+  };


can we have an example here?

do we have circle dependency check somewhere?

No we don't have this check now.

@wanglam if this is not used, we may remove it for now. This helps reduce the scope of the PR and make the code review faster. We can add it back in the future when it's needed.

Removeed this in current PR. Will add it back after requirements clear.

Signed-off-by: Lin Wang <wonglam@amazon.com>

wanglam · 2024-03-13T05:51:36Z

@SuZhou-Joe @Hailong-am All the comments has been addressed, feel free to help me review it. Thank you.

Hailong-am · 2024-03-13T06:19:59Z

src/plugins/workspace/public/components/utils/feature.ts

+        featureIds
+      ),
+    ])
+  );


does this Map featureDependencies contains all dependencies for given feature? for example feature A depends on B and B depends on C, does the featureDependencies[A] will be B and C?

The featureDependencies map was generated by generateFeatureDependencyMap below. The dependencies map generation only use first level data. I think we've already talk about this issue last year.

I see. one level should enough.

Hailong-am · 2024-03-13T06:21:17Z

src/core/public/application/types.ts

+    [key: string]: {
+      type: 'required' | 'optional';
+    };
+  };


do we have circle dependency check somewhere?

ruanyl · 2024-03-13T08:57:49Z

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

+            onChange={handleDefaultVISThemeChange}
+            data-test-subj="workspaceForm-workspaceDetails-defaultVISThemeSelector"
+          />
+        </EuiFormRow>


I may suggest to remove vis theme, color and icon when it's unclear how these will be used.

The defaultVisTheme, icon and dependencies logic has been removed. They are all unclear for now. The color has been used in the dropdown list. Will keep it.

Signed-off-by: Lin Wang <wonglam@amazon.com>

ruanyl · 2024-03-13T09:19:25Z

src/plugins/workspace/common/constants.ts

@@ -7,6 +7,8 @@ export const WORKSPACE_CREATE_APP_ID = 'workspace_create';
 export const WORKSPACE_LIST_APP_ID = 'workspace_list';
 export const WORKSPACE_UPDATE_APP_ID = 'workspace_update';
 export const WORKSPACE_OVERVIEW_APP_ID = 'workspace_overview';
+// These features will be checked and disabled in checkbox on default.


Suggested change

// These features will be checked and disabled in checkbox on default.

// These features will be checked by default in create workspace page and do not allow to edit by the user.

ruanyl · 2024-03-13T09:21:36Z

src/plugins/workspace/common/constants.ts

@@ -7,6 +7,8 @@ export const WORKSPACE_CREATE_APP_ID = 'workspace_create';
 export const WORKSPACE_LIST_APP_ID = 'workspace_list';
 export const WORKSPACE_UPDATE_APP_ID = 'workspace_update';
 export const WORKSPACE_OVERVIEW_APP_ID = 'workspace_overview';
+// These features will be checked and disabled in checkbox on default.
+export const DEFAULT_CHECKED_FEATURES_IDS = [WORKSPACE_UPDATE_APP_ID, WORKSPACE_OVERVIEW_APP_ID];


Nit: DEFAULT_CHECKED_FEATURES_IDS is only used by workspace_form.tsx, consider to move it there. We can move it here in the future if this variable will need to be shared across files.

* Add workspace create page Signed-off-by: Lin Wang <wonglam@amazon.com> * Address PR comments Signed-off-by: Lin Wang <wonglam@amazon.com> * Add more comments Signed-off-by: Lin Wang <wonglam@amazon.com> * Add example for dependencies field in App Signed-off-by: Lin Wang <wonglam@amazon.com> * Separate workspace feature selector Signed-off-by: Lin Wang <wonglam@amazon.com> * Correct example for dependencies Signed-off-by: Lin Wang <wonglam@amazon.com> * Remove unclear icon and defaultVISTheme input Signed-off-by: Lin Wang <wonglam@amazon.com> * Remove unclear dependencies feature Signed-off-by: Lin Wang <wonglam@amazon.com> * Remove states and fix onChange fired after mount Signed-off-by: Lin Wang <wonglam@amazon.com> --------- Signed-off-by: Lin Wang <wonglam@amazon.com>

github-actions bot added the all-star-contributor label Mar 6, 2024

wanglam force-pushed the feat-add-workspace-create-page-offcial-PR branch from bb8692f to baa72cf Compare March 6, 2024 10:26

wanglam marked this pull request as ready for review March 6, 2024 11:26

wanglam requested a review from SuZhou-Joe as a code owner March 6, 2024 11:26

wanglam force-pushed the feat-add-workspace-create-page-offcial-PR branch from baa72cf to e6c443f Compare March 8, 2024 04:36

SuZhou-Joe reviewed Mar 11, 2024

View reviewed changes

wanglam added 3 commits March 12, 2024 16:58

Add workspace create page

9b811da

Signed-off-by: Lin Wang <wonglam@amazon.com>

Address PR comments

1f173f2

Signed-off-by: Lin Wang <wonglam@amazon.com>

Add more comments

2a12e43

Signed-off-by: Lin Wang <wonglam@amazon.com>

wanglam force-pushed the feat-add-workspace-create-page-offcial-PR branch from ac1cc52 to 2a12e43 Compare March 12, 2024 08:58

Hailong-am reviewed Mar 12, 2024

View reviewed changes

wanglam added 3 commits March 13, 2024 10:00

Add example for dependencies field in App

06de67c

Signed-off-by: Lin Wang <wonglam@amazon.com>

Separate workspace feature selector

70096ba

Signed-off-by: Lin Wang <wonglam@amazon.com>

Correct example for dependencies

6b0507b

Signed-off-by: Lin Wang <wonglam@amazon.com>

Hailong-am approved these changes Mar 13, 2024

View reviewed changes

Hailong-am reviewed Mar 13, 2024

View reviewed changes

ruanyl reviewed Mar 13, 2024

View reviewed changes

wanglam added 3 commits March 13, 2024 17:33

Remove unclear icon and defaultVISTheme input

6794393

Signed-off-by: Lin Wang <wonglam@amazon.com>

Remove unclear dependencies feature

582ea9e

Signed-off-by: Lin Wang <wonglam@amazon.com>

Remove states and fix onChange fired after mount

a85666c

Signed-off-by: Lin Wang <wonglam@amazon.com>

ruanyl approved these changes Mar 15, 2024

View reviewed changes

wanglam merged commit 540114b into ruanyl:workspace-pr-integr Mar 15, 2024
43 of 45 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add workspace create page #284

Add workspace create page #284

wanglam commented Mar 6, 2024

codecov-commenter commented Mar 6, 2024 •

edited

Loading

SuZhou-Joe left a comment •

edited

Loading

SuZhou-Joe Mar 11, 2024

SuZhou-Joe Mar 11, 2024

SuZhou-Joe Mar 11, 2024 •

edited

Loading

wanglam Mar 12, 2024

SuZhou-Joe Mar 11, 2024 •

edited

Loading

wanglam Mar 12, 2024

SuZhou-Joe Mar 12, 2024

wanglam Mar 12, 2024 •

edited

Loading

SuZhou-Joe Mar 11, 2024

wanglam Mar 12, 2024

SuZhou-Joe Mar 12, 2024

SuZhou-Joe Mar 11, 2024

SuZhou-Joe Mar 11, 2024

Hailong-am Mar 12, 2024

Hailong-am Mar 13, 2024

wanglam Mar 13, 2024

ruanyl Mar 13, 2024

wanglam Mar 13, 2024

wanglam commented Mar 13, 2024

Hailong-am Mar 13, 2024

wanglam Mar 13, 2024

Hailong-am Mar 13, 2024

Hailong-am Mar 13, 2024

ruanyl Mar 13, 2024

wanglam Mar 13, 2024

ruanyl Mar 13, 2024

ruanyl Mar 13, 2024

	// These features will be checked and disabled in checkbox on default.
	// These features will be checked by default in create workspace page and do not allow to edit by the user.

Add workspace create page #284

Add workspace create page #284

Conversation

wanglam commented Mar 6, 2024

Description

Issues Resolved

Screenshot

Testing the changes

Check List

codecov-commenter commented Mar 6, 2024 • edited Loading

Codecov Report

SuZhou-Joe left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuZhou-Joe Mar 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuZhou-Joe Mar 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wanglam Mar 12, 2024 • 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

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

wanglam commented Mar 13, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 6, 2024 •

edited

Loading

SuZhou-Joe left a comment •

edited

Loading

SuZhou-Joe Mar 11, 2024 •

edited

Loading

SuZhou-Joe Mar 11, 2024 •

edited

Loading

wanglam Mar 12, 2024 •

edited

Loading