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

[ML] Use documentation link service in more ML pages #87389

Merged
merged 4 commits into from
Jan 11, 2021

Conversation

lcawl
Copy link
Contributor

@lcawl lcawl commented Jan 5, 2021

Summary

Replaces more hard-coded documentation links in the machine learning app with keywords from the documentation link service

@lcawl lcawl added :ml v7.12.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Jan 6, 2021
@lcawl lcawl marked this pull request as ready for review January 6, 2021 16:32
@lcawl lcawl requested a review from a team as a code owner January 6, 2021 16:32
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

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 changes here and LGTM. Added a question about the format of the help link text.

Also, I did a quick search on where we use .html in the code, and the only other hard-coded links I found were in ml/common/constants/messages.ts which are for the job validation links:

image

Not sure if you wanted to include those too?

href={`${ELASTIC_WEBSITE_URL}guide/en/elasticsearch/reference/${DOC_LINK_VERSION}/indices-create-index.html#indices-create-index`}
target="_blank"
>
<EuiLink href={createIndexLink} 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.

Just wondering, where we use an EuiLink in this way, should the text end with a . ? Some places we end with a . and some we don't.

image

I prefer without the .

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.

There's an example of "Learn more" with an ending period in https://elastic.github.io/eui/#/guidelines/writing and a quick chat with @gchaps indicates it's preferred in general. I will open a separate PR to get consistency on that.

@lcawl
Copy link
Contributor Author

lcawl commented Jan 7, 2021

@elasticmachine merge upstream

@lcawl lcawl mentioned this pull request Jan 8, 2021
8 tasks
@lcawl
Copy link
Contributor Author

lcawl commented Jan 8, 2021

Also, I did a quick search on where we use .html in the code, and the only other hard-coded links I found were in ml/common/constants/messages.ts which are for the job validation links...
Not sure if you wanted to include those too?

Thanks! I've started investigating fixes for those links in #87763

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.

LGTM

@lcawl
Copy link
Contributor Author

lcawl commented Jan 11, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

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 -1.3KB

History

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:ml release_note:skip Skip the PR/issue when compiling release notes v7.12.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants