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.

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

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

Merged
merged 10 commits into from
Jul 22, 2024

Conversation

Hailong-am
Copy link
Collaborator

@Hailong-am Hailong-am commented Jul 15, 2024

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

Copy link

codecov bot commented Jul 15, 2024

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

Signed-off-by: Hailong Cui <[email protected]>
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
Signed-off-by: Hailong Cui <[email protected]>
private loadWorkspaceListItems() {
if (this.state.filter === 'viewed') {
this.setState({
workspaceList: _.orderBy(this.state.recentWorkspaces, ['timestamp'], ['desc'])
Copy link
Member

@ruanyl ruanyl Jul 21, 2024

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
Collaborator Author

@Hailong-am Hailong-am Jul 21, 2024

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
Collaborator 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
) {
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
Collaborator 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
</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
* 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>
SuZhou-Joe pushed a commit that referenced this pull request Jul 22, 2024
…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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants