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

Add a learn more flyout to the collaborators page #9145

Merged
merged 9 commits into from
Jan 10, 2025

Conversation

Kapian1234
Copy link
Contributor

Description

Add a "Learn More" flyout providing additional information to the collaborators page.

Issues Resolved

Screenshot

Screenshot 2025-01-06 at 16 45 57
Screenshot 2025-01-06 at 16 46 19

Testing the changes

Changelog

  • feat: Add a "Learn More" flyout providing additional information to the collaborators 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 Jan 6, 2025

Codecov Report

Attention: Patch coverage is 72.72727% with 3 lines in your changes missing coverage. Please review.

Project coverage is 60.97%. Comparing base (d947bd6) to head (64476fe).
Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
...orkspace_collaborators/workspace_collaborators.tsx 40.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9145      +/-   ##
==========================================
- Coverage   60.97%   60.97%   -0.01%     
==========================================
  Files        3812     3813       +1     
  Lines       91353    91369      +16     
  Branches    14433    14436       +3     
==========================================
+ Hits        55703    55711       +8     
- Misses      32096    32101       +5     
- Partials     3554     3557       +3     
Flag Coverage Δ
Linux_1 29.06% <72.72%> (+<0.01%) ⬆️
Linux_2 56.45% <ø> (ø)
Linux_3 37.97% <ø> (+<0.01%) ⬆️
Linux_4 29.03% <ø> (-0.01%) ⬇️
Windows_1 29.07% <72.72%> (+<0.01%) ⬆️
Windows_2 56.40% <ø> (ø)
Windows_3 37.97% <ø> (+<0.01%) ⬆️
Windows_4 29.03% <ø> (-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.

];
return (
<EuiFlyout
style={{ maxWidth: 431 }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it necessary to hard code max width here?

Copy link
Contributor Author

@Kapian1234 Kapian1234 Jan 7, 2025

Choose a reason for hiding this comment

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

I found that the maxWidth of use case flyout is 431, so I thought it might be the same for the privacy flyout. We could remove it if it's not necessary.
Screenshot 2025-01-07 at 15 33 47

Copy link
Member

Choose a reason for hiding this comment

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

Flyout has size property, perhaps using s is good enough?

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! size="s" looks better.

Signed-off-by: Kapian1234 <[email protected]>
ruanyl
ruanyl previously approved these changes Jan 8, 2025
Signed-off-by: Kapian1234 <[email protected]>
Comment on lines 110 to 116
{
label: i18n.translate('workspace.form.panels.collaborator.learnMore', {
defaultMessage: 'Learn more',
}),
controlType: 'link',
run: handleLearnMoreClick,
} as TopNavControlLinkData,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be part of description?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Thank you for the reminder!

Comment on lines +19 to +21
it('should render normally', () => {
const { renderResult } = setup();
expect(renderResult).toMatchSnapshot();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: this is too general

{description}
</EuiText>
<EuiSpacer />
<EuiLink href={link} target="_blank" style={{ fontWeight: 'normal' }}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need inline style here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, otherwise it would be bold, which is not expected.

@@ -437,8 +437,10 @@ export class DocLinksService {
advancedSettings: `${OPENSEARCH_DASHBOARDS_VERSIONED_DOCS}management/advanced-settings/`,
},
workspace: {
// https://opensearch.org/docs/latest/dashboards/workspace/workspace-acl/
acl: `${OPENSEARCH_DASHBOARDS_VERSIONED_DOCS}workspace/workspace-acl/`,
Copy link
Member

Choose a reason for hiding this comment

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

Is it safe to delete this entry?

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 was a mistake. It should be removed in this pr but I forgot it. So I remove it here.

@ruanyl ruanyl merged commit e401613 into opensearch-project:main Jan 10, 2025
69 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jan 10, 2025
* Add a learn more flyout to the collaborators page

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

* Changeset file for PR #9145 created/updated

* Update the link

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

* Update snapshots

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

* Modify flyout size

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

* Update collaborators description

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

* Move learn_more link into the description

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

---------

Signed-off-by: Kapian1234 <[email protected]>
Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
(cherry picked from commit e401613)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Jan 10, 2025
…#9145)

* Add a learn more flyout to the collaborators page

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

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

* Update the link

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

* Update snapshots

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

* Modify flyout size

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

* Update collaborators description

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

* Move learn_more link into the description

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

---------

Signed-off-by: Kapian1234 <[email protected]>
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.

5 participants