hide datasource and settings menu when workspace enabled and entering workspace #325
Conversation
src/plugins/management/public/components/management_app/management_app.tsx
Outdated
Show resolved
Hide resolved
| @@ -79,8 +82,19 @@ export const ManagementApp = ({ dependencies, history }: ManagementAppProps) => | |||
| ); | |||
|
|
|||
| useEffect(() => { | |||
| // If workspace is enabled and user has entered workspace, hide advance settings and dataSource menu | |||
| if (capabilities.workspaces.enabled && workspaces.currentWorkspace$.getValue()) { | |||
There was a problem hiding this comment.
Is it safely to access capabilities.workspaces.enabled? what if workspaces is undefined/null?
There was a problem hiding this comment.
As in type definition workspaces capabilities is required. Should we update?
There was a problem hiding this comment.
Just a call out, I am just wondering if workspaces will still be a object if workspaces feature flag is turned off.
There was a problem hiding this comment.
Yes. I checked that there are some other places using this way. https://github.com/ruanyl/OpenSearch-Dashboards/pull/322/files#diff-47d724c5a7bb6e8c1286f67e3ed1b117ac27e05387052760388bd477bd76337bL184
I am thinking that should we maintain to keep capabilities.workspaces.enabled existing and its value should be false if workspace is turned off?
There was a problem hiding this comment.
BTW, if we put this logic in workspace plugin, we don't need to judge if workspace is enabled.
| @@ -79,8 +82,19 @@ export const ManagementApp = ({ dependencies, history }: ManagementAppProps) => | |||
| ); | |||
|
|
|||
| useEffect(() => { | |||
| // If workspace is enabled and user has entered workspace, hide advance settings and dataSource menu | |||
There was a problem hiding this comment.
The change should happen inside workspace directory I suppose?
There was a problem hiding this comment.
Im okay, but if we put it in workspace plugin, we may need to declare to require management plugin in workspace plugin to get management.sections.section.opensearchDashboards, not sure if it's okay.
There was a problem hiding this comment.
That's OK, workspaces has dependency on saved objects management page as well.
Hide data source and advanced settings seems to be a workspace specific feature and better to change inside workspace plugin.
There was a problem hiding this comment.
Makes sense. Then I will add a required declaration.
There was a problem hiding this comment.
shouldn't be optional dependency?
There was a problem hiding this comment.
I think we can use required. As this is an original required plugin, and mark it as required would not have any influences.
There was a problem hiding this comment.
this is an original required plugin
is it management a newly added dependency? let's say if management plugin is not installed somehow, that should not block workspace plugin setup and start.
There was a problem hiding this comment.
As management is a plugin in OSD, it will always be installed?
There was a problem hiding this comment.
@Hailong-am Although management will always be installed, marking it as optional looks more safety and normative, i will keep it the same as savedObjectsManagement and mark it as optional, thanks for deepping dive.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## workspace-pr-integr #325 +/- ##
========================================================
+ Coverage 35.17% 53.58% +18.41%
========================================================
Files 1885 2236 +351
Lines 36421 43530 +7109
Branches 6672 8090 +1418
========================================================
+ Hits 12810 23325 +10515
+ Misses 22761 18561 -4200
- Partials 850 1644 +794
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: tygao <tygao@amazon.com>
…ment_app.tsx Co-authored-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: tygao <tygao@amazon.com>
raintygao
force-pushed
the
feat-hide-menu
branch
from
793bbe6
to
5db6063
Compare
| @@ -118,6 +123,13 @@ export class WorkspacePlugin implements Plugin<{}, {}> { | |||
| currentAppIdSubscription.unsubscribe(); | |||
| }); | |||
| })(); | |||
|
|
|||
| // If workspace is enabled and user has entered workspace, hide advance settings and dataSource menu and disable | |||
There was a problem hiding this comment.
Nit: better to extract to a private method and listen the currentWorkspaceId$ change. currentWorkspaceId$ should be the single source of truth for detecting whether inside a workspace.
Signed-off-by: tygao <tygao@amazon.com>
Signed-off-by: tygao <tygao@amazon.com>
Signed-off-by: tygao <tygao@amazon.com>
| */ | ||
| private disableManagementApps(core: CoreSetup, management: ManagementSetup) { | ||
| const currentWorkspaceId$ = core.workspaces.currentWorkspaceId$; | ||
| this.managementCurrentWorkspaceIdSubscription?.unsubscribe(); |
There was a problem hiding this comment.
can we move this to stop method?
There was a problem hiding this comment.
Stop method will also call this.
Co-authored-by: Yulong Ruan <ruanyu1@gmail.com>
Signed-off-by: tygao <tygao@amazon.com>
…rch-Dashboards into feat-hide-menu
…rch-Dashboards into feat-hide-menu Signed-off-by: tygao <tygao@amazon.com>
Description
hide datasource and settings menu in dashboard management section when workspace is enabled and user has entered workspace.
Issues Resolved
Screenshot
Testing the changes
Check List
yarn test:jestyarn test:jest_integration