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]Refactor use case selector in workspace creation page #8413

Conversation

wanglam
Copy link
Contributor

@wanglam wanglam commented Oct 1, 2024

Description

This PR is for refactoring the use case selector in the workspace creation page. It mainly refactors the features displayed in the use case card. The feature descriptions are moved to a separate use case flyout, which will be displayed after clicking the 'Learn more' button.

Screenshot

image
image

Testing the changes

  • Clone branch code and run yarn osd bootstrap --single-version loose
  • Add below configs in config/opensearch_dashboards.yml
savedObjects.permission.enabled: true
workspace.enabled: true
uiSettings:
  overrides:
    'home:useNewHomePage': true
opensearchDashboards.dashboardAdmin.users: ['admin']
  • Run yarn start --no-base-path
  • Login with admin user and go to workspace create page by bottom left menu
  • The use case selector panel should be displayed like screenshot above
  • The flyout should be displayed after "Learn more" button clicked

Changelog

  • feat: [Workspace]Refactor use case selector in workspace creation 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

Signed-off-by: Lin Wang <wonglam@amazon.com>
Signed-off-by: Lin Wang <wonglam@amazon.com>
Signed-off-by: Lin Wang <wonglam@amazon.com>
Signed-off-by: Lin Wang <wonglam@amazon.com>
Copy link

codecov bot commented Oct 1, 2024

Codecov Report

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

Project coverage is 60.94%. Comparing base (26dca19) to head (0ba9f44).
Report is 133 commits behind head on main.

Files with missing lines Patch % Lines
src/plugins/workspace/public/utils.ts 95.45% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8413      +/-   ##
==========================================
+ Coverage   60.93%   60.94%   +0.01%     
==========================================
  Files        3757     3758       +1     
  Lines       89263    89285      +22     
  Branches    13960    13968       +8     
==========================================
+ Hits        54396    54419      +23     
+ Misses      31479    31478       -1     
  Partials     3388     3388              
Flag Coverage Δ
Linux_1 28.90% <97.91%> (+0.02%) ⬆️
Linux_2 56.35% <ø> (ø)
Linux_3 37.78% <ø> (+<0.01%) ⬆️
Linux_4 29.96% <ø> (ø)
Windows_1 28.91% <97.91%> (+0.02%) ⬆️
Windows_2 56.30% <ø> (ø)
Windows_3 37.78% <ø> (ø)
Windows_4 29.96% <ø> (ø)

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.

Copy link
Member

@ruanyl ruanyl left a comment

Choose a reason for hiding this comment

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

Left a few minor comment

&nbsp;
<EuiLink onClick={handleLearnMoreClick}>
{i18n.translate('workspace.form.panels.useCase.learnMore', {
defaultMessage: 'Learn more.',
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
defaultMessage: 'Learn more.',
defaultMessage: 'Learn more',

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
Add "." to align with UX design.

{features && features.length > 0 && (
<>
{i18n.translate('workspace.forms.useCaseFlyout.featuresIncluded', {
defaultMessage: 'Features included:',
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
defaultMessage: 'Features included:',
defaultMessage: 'Features included',

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
This ":" is for align with UX design.

i18n.translate(
'workspace.forms.useCaseFlyout.featuresDetails.delimiter',
{
defaultMessage: ', ',
Copy link
Member

Choose a reason for hiding this comment

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

Just curious, do we need i18n for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this delimiter should have an i18n version, for example the comma in English and Chinese is a different symbol and will have different spacing. I think it would be a better idea using i18n.

@@ -38,10 +38,11 @@ const defaultNavGroups = {
all: {
id: ALL_USE_CASE_ID,
title: i18n.translate('core.ui.group.all.title', {
defaultMessage: 'Analytics (All)',
defaultMessage: 'Analytics (all features)',
Copy link
Collaborator

Choose a reason for hiding this comment

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

The use case name might be Analytic
image

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
They are using "Analytics (all features)" in the use case card side. Let me customized the use case name here. Will confirm with UX, shall we using different name in other places.

ruanyl
ruanyl previously approved these changes Oct 1, 2024
@wanglam
Copy link
Contributor Author

wanglam commented Oct 2, 2024

@Hailong-am I've removed the (all features) suffix. Could you help me take a look?

@@ -133,29 +103,68 @@ export const WorkspaceUseCase = ({
formErrors,
availableUseCases,
}: WorkspaceUseCaseProps) => {
const [isFlyoutVisible, setIsFlyoutVisible] = useState(false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: could named as isUseCaseFlyoutVisible

}: WorkspaceUseCaseFlyoutProps) => {
return (
<EuiFlyout
style={{ maxWidth: 431 }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

where does the 431 comes from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's from the UX design.

Comment on lines 376 to 378
if (featureId.endsWith('overview')) {
continue;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe remove get started as well?

Hailong-am
Hailong-am previously approved these changes Oct 2, 2024
Signed-off-by: Lin Wang <wonglam@amazon.com>
Signed-off-by: Lin Wang <wonglam@amazon.com>
@SuZhou-Joe SuZhou-Joe merged commit 7b5a379 into opensearch-project:main Oct 2, 2024
66 of 67 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Oct 2, 2024
* Refactor use case selector in workspace create

Signed-off-by: Lin Wang <wonglam@amazon.com>

* Add gap for details panel title

Signed-off-by: Lin Wang <wonglam@amazon.com>

* Update use case name and description

Signed-off-by: Lin Wang <wonglam@amazon.com>

* Filter out hidden nav links

Signed-off-by: Lin Wang <wonglam@amazon.com>

* Changeset file for PR #8413 created/updated

* Add test case for different feature details

Signed-off-by: Lin Wang <wonglam@amazon.com>

* Change back to "Analytics"

Signed-off-by: Lin Wang <wonglam@amazon.com>

* Add all features suffix for all use case

Signed-off-by: Lin Wang <wonglam@amazon.com>

* Fix failed unit tests

Signed-off-by: Lin Wang <wonglam@amazon.com>

* Filter out getting started links

Signed-off-by: Lin Wang <wonglam@amazon.com>

* Renaming isFlyoutVisible to isUseCaseFlyoutVisible

Signed-off-by: Lin Wang <wonglam@amazon.com>

---------

Signed-off-by: Lin Wang <wonglam@amazon.com>
Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
(cherry picked from commit 7b5a379)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
SuZhou-Joe pushed a commit that referenced this pull request Oct 3, 2024
…) (#8444)

* Refactor use case selector in workspace create



* Add gap for details panel title



* Update use case name and description



* Filter out hidden nav links



* Changeset file for PR #8413 created/updated

* Add test case for different feature details



* Change back to "Analytics"



* Add all features suffix for all use case



* Fix failed unit tests



* Filter out getting started links



* Renaming isFlyoutVisible to isUseCaseFlyoutVisible



---------



(cherry picked from commit 7b5a379)

Signed-off-by: Lin Wang <wonglam@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>
SuZhou-Joe pushed a commit to SuZhou-Joe/OpenSearch-Dashboards that referenced this pull request Oct 3, 2024
…nsearch-project#8413) (opensearch-project#8444)

* Refactor use case selector in workspace create



* Add gap for details panel title



* Update use case name and description



* Filter out hidden nav links



* Changeset file for PR opensearch-project#8413 created/updated

* Add test case for different feature details



* Change back to "Analytics"



* Add all features suffix for all use case



* Fix failed unit tests



* Filter out getting started links



* Renaming isFlyoutVisible to isUseCaseFlyoutVisible



---------



(cherry picked from commit 7b5a379)

Signed-off-by: Lin Wang <wonglam@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>
@ananzh ananzh added the v2.18.0 label Oct 30, 2024
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.

5 participants