[Workspace][Page header] Integrate new page header for workspace pages #7697
Conversation
raintygao
requested review from
ananzh,
kavilla,
AMoo-Miki,
ashwin-pc,
joshuarrrr,
abbyhu2000,
zengyan-amazon,
zhongnansu,
manasvinibs,
ZilongX,
Flyingliuhub,
curq,
bandinib-amzn,
SuZhou-Joe,
ruanyl,
BionIT,
xinruiba,
zhyuanqi,
mengweieric,
LDrago27,
virajsanghvi,
sejli and
joshuali925
as code owners
❌ Changelog Entry Missing HyphenChangelog entries must begin with a hyphen (-). |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7697 +/- ##
==========================================
- Coverage 63.74% 63.74% -0.01%
==========================================
Files 3637 3637
Lines 80448 80445 -3
Branches 12792 12791 -1
==========================================
- Hits 51285 51282 -3
+ Misses 26029 26028 -1
- Partials 3134 3135 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| if (!appUrl) return ''; | ||
|
|
||
| return cleanWorkspaceId(appUrl); | ||
| }, [application, http]); |
There was a problem hiding this comment.
Shall we remove the http dependency? Seems not been used in the useMemo body.
There was a problem hiding this comment.
Sure. This is a previous missed change, thanks for catching it. Updated.
| }); | ||
| if (!appUrl) return ''; | ||
|
|
||
| return cleanWorkspaceId(appUrl); |
There was a problem hiding this comment.
Shall we need to cleanWorkspaceId? As my understanding, workspace list is out of workspace, it won't includes workspace id.
There was a problem hiding this comment.
I have forgot why we add this line before. @SuZhou-Joe Seems we can return app url directly for now, right?
There was a problem hiding this comment.
Yeah, no need to cleanWorkspaceId.
| ]} | ||
| setMountPoint={application?.setAppDescriptionControls} | ||
| /> | ||
| {renderCreateWorkspaceButton()} |
There was a problem hiding this comment.
Nit: How about refactor code like below?
{isDashboardAdmin && renderCreateWorkspaceButton()}Then we don't need to check isDashboardAdmin in renderCreateWorkspaceButton.
| @@ -36,7 +36,9 @@ export const WorkspaceDetailApp = (props: WorkspaceDetailProps) => { | |||
| text: currentWorkspace.name, | |||
| }); | |||
| breadcrumbs.push({ | |||
| text: i18n.translate('workspace.detail.breadcrumb', { defaultMessage: 'Workspace Detail' }), | |||
| text: `${currentWorkspace.name} ${i18n.translate('workspace.detail.title', { | |||
There was a problem hiding this comment.
Nit: prefer write i18n like below code:
i18n.translate('workspace.detail.title', {
defaultMessage: "{name} settings",
values: {
name: currentWorkspace.name
}
})| const navLinks = ( | ||
| await getNavLinksByNavGroupId(DEFAULT_NAV_GROUPS.settingsAndSetup.id) | ||
| ).filter((navLink) => navLink.id !== settingsLandingPageId); | ||
|
|
||
| coreStart.chrome.setBreadcrumbs([ |
There was a problem hiding this comment.
Nit: How about move page title update logic to FeatureCards component?
There was a problem hiding this comment.
I would say that I have no preference whether to still maintain it here or move it into component, because I think there won't be much difference on it or maintainability.
Signed-off-by: tygao <tygao@amazon.com>
|
From the screenshot, can we remove the white background of workspace list page? |
Signed-off-by: tygao <tygao@amazon.com>
Sure. Have updated code and screenshot. |
#7697) * init page headers for workspace Signed-off-by: tygao <tygao@amazon.com> * update Signed-off-by: tygao <tygao@amazon.com> * update list page Signed-off-by: tygao <tygao@amazon.com> * Changeset file for PR #7697 created/updated --------- Signed-off-by: tygao <tygao@amazon.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> (cherry picked from commit 36af5e7) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
#7697) (#7717) * init page headers for workspace * update * update list page * Changeset file for PR #7697 created/updated --------- (cherry picked from commit 36af5e7) Signed-off-by: tygao <tygao@amazon.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
Description
Integrate new page header for workspace pages, including
This is first phase of integration, the integration of remaining pages will be proposed in later PRs.
Screenshot
Testing the changes
Changelog
Check List
yarn test:jestyarn test:jest_integration