-
Notifications
You must be signed in to change notification settings - Fork 918
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
[Doc Links Service] Clean up noDocumentation links #3685
Conversation
Thank you for your recent code changes! I noticed that the |
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## main #3685 +/- ##
==========================================
- Coverage 66.43% 66.43% -0.01%
==========================================
Files 3210 3210
Lines 61677 61677
Branches 9522 9522
==========================================
- Hits 40977 40973 -4
- Misses 18419 18422 +3
- Partials 2281 2282 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sayuree Thanks for the PR - it mostly looks great!
I found some additional links that can be defined.
There are also some minor requested changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sayuree Can you also add the commented links to the restAPI
section (L330-340)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For L442-L445, please move visualize
to the opensearchDashboards
section and update visualize.guide
to be https://opensearch.org/docs/latest/dashboards/visualize/viz-index/
@@ -40,7 +40,7 @@ export const ExperimentalCallout = ({ docLinks }: { docLinks: DocLinksStart }) = | |||
defaultMessage="To leave feedback, visit " | |||
/> | |||
} | |||
<EuiLink href={docLinks.links.noDocumentation.openSearchForum} target="_blank"> | |||
<EuiLink href={docLinks.links.opensearch.openSearchForum} target="_blank"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also update the mock in src/plugins/data_source_management/public/mocks.ts
@sayuree Will you be able to make the additional requested changes? |
@joshuarrrr Added requested changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @sayuree , almost there! Just three tasks left, summarized here:
- Can you also add the commented links to the restAPI section (L345-355)?
- Please also update the mock in src/plugins/data_source_management/public/mocks.ts
- See suggested changes for
visualize.guide
link.
guide: `${OPENSEARCH_DASHBOARDS_VERSIONED_DOCS}discover/multi-data-sources/`, | ||
}, | ||
visualize: { | ||
guide: `${OPENSEARCH_WEBSITE_DOCS}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
guide: `${OPENSEARCH_WEBSITE_DOCS}`, | |
// https://opensearch.org/docs/latest/dashboards/visualize/viz-index/ | |
guide: `${OPENSEARCH_WEBSITE_DOCS}visualize/viz-index/`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, looks good, I'll just wait for the automation to complete before approving. Thanks for all the changes!
@sayuree Can you rebase or update the PR to resolve the conflict? I believe the experimental callout has since been removed. |
Signed-off-by: sabina.zaripova <sabina.zaripova@nu.edu.kz>
I have updated the PR, hope changes will be approved. |
…ct#3685) Signed-off-by: sabina.zaripova <sabina.zaripova@nu.edu.kz>
) * Add noDocumentation links (#3208) (#3685) Signed-off-by: sabina.zaripova <sabina.zaripova@nu.edu.kz> (cherry picked from commit 4e14aae) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * update changelog Signed-off-by: Josh Romero <rmerqg@amazon.com> --------- Signed-off-by: sabina.zaripova <sabina.zaripova@nu.edu.kz> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Signed-off-by: Josh Romero <rmerqg@amazon.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Josh Romero <rmerqg@amazon.com>
…ct#3685) Signed-off-by: sabina.zaripova <sabina.zaripova@nu.edu.kz> Signed-off-by: David Sinclair <david@sinclair.tech>
Description
[Added corresponding links from the documentation for non-working links
Edited DocLinksStart interface properties
Edited the usage of interface in other files correspondigly
]
Issues Resolved
fixes #3208
Check List
yarn test:jest
yarn test:jest_integration
yarn test:ftr