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

[workspace]Fix/UI of assets table #8519

Merged
merged 11 commits into from
Dec 24, 2024

Conversation

Qxisylolo
Copy link
Contributor

@Qxisylolo Qxisylolo commented Oct 8, 2024

Description

This pr addresses the UI of assets table by:

  1. Show a couple of workspace names + a badge "+ N more" showing a popover with the rest of the list of the workspaces(To adjust UI and unstable workspace name length, only one workspace name will he shown)
  2. Add "database" icon for the assets of the type "Data source"
  3. Add "Gear" icon to the config type assets
  4. Is there are no workspaces, show mdash in the table cell

Screenshot

截屏2024-10-08 08 10 39 截屏2024-10-30 16 33 19

Changelog

  • fix: adds badge when there are more than one workspaces and updates the icon

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 Oct 9, 2024

Codecov Report

Attention: Patch coverage is 92.85714% with 1 line in your changes missing coverage. Please review.

Project coverage is 60.92%. Comparing base (608911e) to head (8b9dc6e).
Report is 47 commits behind head on main.

Files with missing lines Patch % Lines
...c/components/workspace_column/workspace_column.tsx 92.85% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8519      +/-   ##
==========================================
+ Coverage   60.85%   60.92%   +0.06%     
==========================================
  Files        3802     3808       +6     
  Lines       91062    91190     +128     
  Branches    14376    14399      +23     
==========================================
+ Hits        55419    55554     +135     
+ Misses      32104    32083      -21     
- Partials     3539     3553      +14     
Flag Coverage Δ
Linux_1 29.00% <92.85%> (-0.02%) ⬇️
Linux_2 56.38% <ø> (-0.01%) ⬇️
Linux_3 38.01% <ø> (+0.14%) ⬆️
Linux_4 29.01% <ø> (+0.01%) ⬆️
Windows_1 29.03% <92.85%> (+<0.01%) ⬆️
Windows_2 56.34% <ø> (-0.01%) ⬇️
Windows_3 38.01% <ø> (+0.14%) ⬆️
Windows_4 29.01% <ø> (+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.

@Qxisylolo Qxisylolo force-pushed the fix/UI_of_assets_table branch from 1c905bc to 746db1c Compare October 31, 2024 07:30
opensearch-changeset-bot bot added a commit to Qxisylolo/OpenSearch-Dashboards that referenced this pull request Oct 31, 2024
@@ -283,13 +298,71 @@ export class Table extends PureComponent<TableProps, TableState> {
'data-test-subj': 'updated-at',
render: (updatedAt: string) => updatedAt && moment(updatedAt).format(dateFormat),
} as EuiTableFieldDataColumnType<SavedObjectWithMetadata<any>>,
...columnRegistry.getAll().map((column) => {
Copy link
Member

Choose a reason for hiding this comment

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

We are not supposed to delete the columnRegistry here, it is an extension point for other plugins to register their customized column.

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 the comment, already updated

@Qxisylolo Qxisylolo force-pushed the fix/UI_of_assets_table branch from 223dd7d to ef28e15 Compare November 19, 2024 03:34
@@ -224,7 +223,7 @@ export class Table extends PureComponent<TableProps, TableState> {
<EuiToolTip position="top" content={getSavedObjectLabel(type)}>
<EuiIcon
aria-label={getSavedObjectLabel(type)}
type={object.meta.icon || 'apps'}
type={object.type === 'config' ? 'gear' : object.meta.icon || 'app'}
Copy link
Member

Choose a reason for hiding this comment

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

Why don't we just changed the meta.icon?

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 all the comments, updated.

panelPaddingSize="s"
data-test-subj="workspace-column-popover"
>
{workspaceNames?.slice(1).map((ws) => (
Copy link
Member

Choose a reason for hiding this comment

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

The ws may be undefined, shall we filter those workspace out?

Comment on lines 32 to 34
expect(getByTestId('workspace-column-popover')).toBeInTheDocument();
expect(getByText('foo')).toBeInTheDocument();
expect(getByText('bar')).toBeInTheDocument();
Copy link
Member

Choose a reason for hiding this comment

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

Nit: we could use findByText or findByTestId for such kind of assertion, those commands have retry mechanism out of the box.

Signed-off-by: Qxisylolo <[email protected]>
Signed-off-by: Qxisylolo <[email protected]>
setShowBadgePopover(false);
};

if (workspaceNames) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (workspaceNames) {
if (workspaceNames?.length) {

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 comment, updated

Signed-off-by: Qxisylolo <[email protected]>
ruanyl
ruanyl previously approved these changes Dec 18, 2024
@@ -109,7 +109,6 @@ import { DataPublicPluginStart } from '../../../../../plugins/data/public';
import { DuplicateObject } from '../types';
import { formatWorkspaceIdParams } from '../../utils';
import { NavigationPublicPluginStart } from '../../../../navigation/public';
import { WorkspaceObject } from '../../../../workspaces/public';
Copy link
Collaborator

Choose a reason for hiding this comment

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

why this import been removed?

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 spotting the error already reverted.

Copy link
Member

Choose a reason for hiding this comment

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

Seems it should import from core now:

import { WorkspaceObject } from '../../../../../core/public';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This import is correct, sorry I haven't made a double check. already updated

Copy link
Member

Choose a reason for hiding this comment

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

No problem, I guess the export location was changed in another PR, thanks for fixing this.

@@ -637,7 +637,6 @@ describe('SavedObjectsTable', () => {
{ force: true }
);
expect(component.state('selectedSavedObjects').length).toBe(0);
expect(notifications.toasts.addDanger).not.toHaveBeenCalled();
Copy link
Collaborator

Choose a reason for hiding this comment

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

any reason to remove this assertion?

Copy link
Contributor Author

@Qxisylolo Qxisylolo Dec 24, 2024

Choose a reason for hiding this comment

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

Thanks for spotting the error, I think it is an accidental delete. already reverted.

Signed-off-by: Qxisylolo <[email protected]>
@Qxisylolo Qxisylolo force-pushed the fix/UI_of_assets_table branch from 937fc4e to 9c91505 Compare December 24, 2024 06:23
SuZhou-Joe
SuZhou-Joe previously approved these changes Dec 24, 2024
Signed-off-by: Qxisylolo <[email protected]>
@Hailong-am Hailong-am merged commit 6e851ff into opensearch-project:main Dec 24, 2024
69 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Dec 24, 2024
* fix/UI_of_assets_table

Signed-off-by: Qxisylolo <[email protected]>

* fix/the_UI_of_assets_table, add tests

Signed-off-by: Qxisylolo <[email protected]>

* use workspaceNameIdLookup

Signed-off-by: Qxisylolo <[email protected]>

* delete comment

Signed-off-by: Qxisylolo <[email protected]>

* use workspace column

Signed-off-by: Qxisylolo <[email protected]>

* Changeset file for PR #8519 created/updated

* use gear icon

Signed-off-by: Qxisylolo <[email protected]>

* use gear icon

Signed-off-by: Qxisylolo <[email protected]>

* new update

Signed-off-by: Qxisylolo <[email protected]>

* revert deleted line

Signed-off-by: Qxisylolo <[email protected]>

* correct import

Signed-off-by: Qxisylolo <[email protected]>

---------

Signed-off-by: Qxisylolo <[email protected]>
Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
(cherry picked from commit 6e851ff)
Signed-off-by: github-actions[bot] <github-actions[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