Skip to content

[ML] Adds documentation links in the help menu for machine learning #85366

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

Merged
merged 13 commits into from
Dec 18, 2020

Conversation

lcawl
Copy link
Contributor

@lcawl lcawl commented Dec 9, 2020

Summary

Related to #85088

This PR adds a link to the machine learning guide in the help menu on the "Overview" page in the ML app:

image

The method for accomplishing this drop-down menu link is copied from how it's done in:

If we like this layout, I think we can add appropriate links for each of the other pages in the app.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

Took an early test of this one, and great to see the ML content in the Help menu! Left a few comments on the code.

useEffect(() => {
chrome.setHelpExtension({
appName: i18n.translate('xpack.ml.chrome.help.appName', {
defaultMessage: 'Machine learning',
Copy link
Contributor

Choose a reason for hiding this comment

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

Would capitalized Learning be appropriate here, to match the display of the ML plugin in the top and side nav, and also the title on the doc page Machine Learning in the Elastic Stack?

content: i18n.translate('xpack.ml.chrome.helpMenu.documentation', {
defaultMessage: 'Documentation',
}),
href: docLinks.links.ml.guide,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we always want to link to the top level ML guide from every page in the ML app, or should the href be passed in to the HelpMenu component?

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, I was thinking it would be nice to have that functionality, but wasn't sure of the best way to accomplish that. I'll maybe reach out to pick your brain on the right code to accomplish it.


import React, { useEffect } from 'react';
import { i18n } from '@kbn/i18n';
import { useKibana } from '../../../../../../../src/plugins/kibana_react/public';
Copy link
Contributor

Choose a reason for hiding this comment

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

@jgowdyelastic would it be better to use useMlKibana for access to the docLinks service here?

Copy link
Member

Choose a reason for hiding this comment

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

for consistency with our other components, yes useMlKibana would be better.
but chrome and docLinks are available in either.

@lcawl lcawl added enhancement New value added to drive a business result v7.12.0 labels Dec 17, 2020
@lcawl lcawl marked this pull request as ready for review December 17, 2020 00:32
@lcawl lcawl requested a review from a team as a code owner December 17, 2020 00:32
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@lcawl
Copy link
Contributor Author

lcawl commented Dec 17, 2020

@elasticmachine merge upstream

@alvarezmelissa87
Copy link
Contributor

Great addition, @lcawl! 😄

Might be worth renaming the files so that the HelpText component lives in help_text.tsx in the help_text/ directory and then it is exported in an index.ts file (in the same directory) like export { HelpText } from './help_text
The import statement for HelpText wouldn't have to change as it checks for an index file by default.

This would keep it consistent with the other components in the plugin.

href: docLink,
iconType: 'documents',
linkType: 'custom',
target: '_blank',
Copy link
Contributor

Choose a reason for hiding this comment

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

Despite setting target to _blank, the link opens in the same tab for me. Same thing happens with the links for the Security solution I see. But for Dashboard, it opens in a different tab (which seems a better option). I can't work out why that it is, as the structure of the <a> tag looks identical.

Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 Dec 17, 2020

Choose a reason for hiding this comment

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

@lcawl Looks like the linkType here needs to be 'documentation'. That should fix the issue.
https://github.com/elastic/kibana/blob/master/src/core/public/chrome/ui/header/header_help_menu.tsx#L287

docLink: string;
}

export const HelpMenu: FC<HelpMenuProps> = React.memo(({ docLink }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

To be consistent with the other components in the ml/public/application/components folder, it is probably better to move this content into help_menu.tsx and then create a simple index.tsx which exports out HelpMenu (copy structure of ml/public/application/components/create_job_link_card).

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
ml 1688 1690 +2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
ml 7.0MB 7.0MB +642.0B

Distributable file count

id before after diff
default 47287 48047 +760

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

Tested latest edits and LGTM. Thanks for setting this up!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants