Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Navigation-next]Register workspace list card into home page #7247

Merged
merged 10 commits into from
Jul 22, 2024
Merged

[Navigation-next]Register workspace list card into home page #7247

merged 10 commits into from
Jul 22, 2024

Conversation

Hailong-am
Copy link
Contributor

@Hailong-am Hailong-am commented Jul 15, 2024 •
edited
Loading

Description

Register workspace list card into home page

image

Empty state

image

Issues Resolved

#7294

Screenshot

Testing the changes

  1. start OSD with workspace and new navigation enabled, add this to opensearch_dashboards.yml file
workspace.enabled: true
uiSettings:
  overrides:
    "home:useNewHomePage": true
  1. start OSD and navigates to home page

Changelog

  • feat: Register workspace list card into home page

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

@github-actions github-actions bot added seasoned-contributor Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry labels Jul 15, 2024
@codecov Codecov
Copy link

codecov bot commented Jul 15, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 58.53659% with 17 lines in your changes missing coverage. Please review.

Project coverage is 67.70%. Comparing base (376ead0) to head (5eb1202).

Files Patch % Lines
...ic/components/service_card/workspace_list_card.tsx 62.85% 9 Missing and 4 partials ⚠️
src/plugins/workspace/public/plugin.ts 33.33% 3 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #7247      +/-   ##
==========================================
- Coverage   67.72%   67.70%   -0.02%     
==========================================
  Files        3518     3519       +1     
  Lines       69638    69679      +41     
  Branches    11365    11373       +8     
==========================================
+ Hits        47165    47179      +14     
- Misses      19684    19706      +22     
- Partials     2789     2794       +5
Flag Coverage Δ
Linux_1 33.18% <58.53%> (+0.02%) ⬆️
Linux_2 55.46% <ø> (ø)
Linux_3 43.31% <ø> (ø)
Linux_4 34.69% <ø> (ø)
Windows_1 33.20% <58.53%> (-0.01%) ⬇️
Windows_2 55.41% <ø> (ø)
Windows_3 43.33% <ø> (ø)
Windows_4 34.69% <ø> (+<0.01%) ⬆️

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.

@Hailong-am Hailong-am marked this pull request as draft July 16, 2024 05:05
@SuZhou-Joe SuZhou-Joe force-pushed the feature/navigation-next branch 2 times, most recently from d73444b to eaf5fea Compare July 17, 2024 07:26
@ruanyl
Copy link
Member

ruanyl commented Jul 20, 2024

Looks like unnecessary commits are included, maybe you can do a cherry pick instead

@Hailong-am
workspace list card on home
2e400e4 
Signed-off-by: Hailong Cui <[email protected]>
@Hailong-am
fix merge conflicts
2897183 
Signed-off-by: Hailong Cui <[email protected]>
@github-actions github-actions bot removed the Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry label Jul 21, 2024
@Hailong-am
fix failed UT
9a5846d 
Signed-off-by: Hailong Cui <[email protected]>
ruanyl
ruanyl reviewed Jul 21, 2024
View reviewed changes
src/plugins/workspace/public/components/service_card/workspace_list_card.tsx Outdated Show resolved Hide resolved
src/plugins/workspace/public/components/service_card/workspace_list_card.tsx Outdated Show resolved Hide resolved
src/plugins/workspace/public/components/service_card/workspace_list_card.tsx Outdated
private loadWorkspaceListItems() {
if (this.state.filter === 'viewed') {
this.setState({
workspaceList: _.orderBy(this.state.recentWorkspaces, ['timestamp'], ['desc'])
Copy link
Member

@ruanyl ruanyl Jul 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems workspaceList can be derived from existing state, I think we can simply compute it in the render() function.

If you use functional react component, you can use useMemo to avoid unnecessary recompute. Btw, why not use functional component? I can see it will make the implementation much simpler

Copy link
Contributor Author

@Hailong-am Hailong-am Jul 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, why not use functional component? I can see it will make it implementation much simple

when i try to use functional component, dashboard embeddable not able to render it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, that's odd, then I think you can keep the current implementation, I will investigate a bit on this, thanks ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks to @ruanyl help on debugging, seems functional component don't any issue, convert to functional component now.

ruanyl
ruanyl previously approved these changes Jul 21, 2024
View reviewed changes
SuZhou-Joe
SuZhou-Joe reviewed Jul 21, 2024
View reviewed changes
src/plugins/workspace/public/plugin.ts Outdated
) {
if (contentManagement) {
contentManagement.registerContentProvider({
id: HOME_PAGE_ID,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the id is used for the content, not the page.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is the content provider id, you can give it any unique string, like workspace_list_card

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for catching this, update to workspace_list_card_home

SuZhou-Joe
SuZhou-Joe previously approved these changes Jul 21, 2024
View reviewed changes
ruanyl
ruanyl reviewed Jul 21, 2024
View reviewed changes
src/plugins/workspace/public/components/service_card/workspace_list_card.tsx
</EuiFlexGroup>

<EuiSpacer />
<EuiListGroup flush={true} bordered={false} style={{ minHeight: '300px' }}>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I'd suggest to have a future improvement task make this service card a flex container, so we don't need to specify a fix number 300px to stretch the list.

@SuZhou-Joe SuZhou-Joe merged commit b32eade into opensearch-project:main Jul 22, 2024
65 of 67 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jul 22, 2024
@github-actions @opensearch-changeset-bot
[Navigation-next]Register workspace list card into home page (#7247)
c022a87 
* workspace list card on home

Signed-off-by: Hailong Cui <[email protected]>

* fix merge conflicts

Signed-off-by: Hailong Cui <[email protected]>

* add home as requiredBundles

Signed-off-by: Hailong Cui <[email protected]>

* Changeset file for PR #7247 created/updated

* fix failed UT

Signed-off-by: Hailong Cui <[email protected]>

* address review comments

Signed-off-by: Hailong Cui <[email protected]>

* update to funtional component

Signed-off-by: Hailong Cui <[email protected]>

* udpate content provider id

Signed-off-by: Hailong Cui <[email protected]>

---------

Signed-off-by: Hailong Cui <[email protected]>
Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
(cherry picked from commit b32eade)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Jul 22, 2024
Merged
SuZhou-Joe pushed a commit that referenced this pull request Jul 22, 2024
@opensearch-trigger-bot @github-actions @opensearch-changeset-bot
[Navigation-next]Register workspace list card into home page (#7247) (#…
8a6da4c 
…7350)

* workspace list card on home



* fix merge conflicts



* add home as requiredBundles



* Changeset file for PR #7247 created/updated

* fix failed UT



* address review comments



* update to funtional component



* udpate content provider id



---------



(cherry picked from commit b32eade)

Signed-off-by: Hailong Cui <[email protected]>
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ruanyl ruanyl ruanyl approved these changes

@SuZhou-Joe SuZhou-Joe SuZhou-Joe approved these changes

@ananzh ananzh Awaiting requested review from ananzh ananzh is a code owner

@kavilla kavilla Awaiting requested review from kavilla kavilla is a code owner

@AMoo-Miki AMoo-Miki Awaiting requested review from AMoo-Miki AMoo-Miki is a code owner

@ashwin-pc ashwin-pc Awaiting requested review from ashwin-pc ashwin-pc is a code owner

@joshuarrrr joshuarrrr Awaiting requested review from joshuarrrr joshuarrrr is a code owner

@abbyhu2000 abbyhu2000 Awaiting requested review from abbyhu2000 abbyhu2000 is a code owner

@zengyan-amazon zengyan-amazon Awaiting requested review from zengyan-amazon zengyan-amazon is a code owner

@zhongnansu zhongnansu Awaiting requested review from zhongnansu zhongnansu is a code owner

@manasvinibs manasvinibs Awaiting requested review from manasvinibs manasvinibs is a code owner

@ZilongX ZilongX Awaiting requested review from ZilongX ZilongX is a code owner

@Flyingliuhub Flyingliuhub Awaiting requested review from Flyingliuhub Flyingliuhub is a code owner

@curq curq Awaiting requested review from curq curq is a code owner

@bandinib-amzn bandinib-amzn Awaiting requested review from bandinib-amzn bandinib-amzn is a code owner

@BionIT BionIT Awaiting requested review from BionIT BionIT is a code owner

@xinruiba xinruiba Awaiting requested review from xinruiba xinruiba is a code owner

@zhyuanqi zhyuanqi Awaiting requested review from zhyuanqi zhyuanqi is a code owner

@mengweieric mengweieric Awaiting requested review from mengweieric mengweieric is a code owner

@LDrago27 LDrago27 Awaiting requested review from LDrago27 LDrago27 is a code owner

@virajsanghvi virajsanghvi Awaiting requested review from virajsanghvi virajsanghvi is a code owner

Assignees
No one assigned
Labels
backport 2.x seasoned-contributor v2.16.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Hailong-am @virajsanghvi @ruanyl @SuZhou-Joe