-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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 Vega help link to DocLinksService #87721
Conversation
This link is currently wrong, but I'm testing to make sure it will get caught if it's included in this file. A future commit will actually use this variables in vega_help_menu.tsx, and update the link in `doc_links_service.ts` to be correct. This is part of a fix for elastic#87367 Related: elastic/docs#1805
Pinging @elastic/kibana-docs (Team:Docs) |
@@ -167,6 +167,7 @@ export class DocLinksService { | |||
timelionDeprecation: `${ELASTIC_WEBSITE_URL}guide/en/kibana/${DOC_LINK_VERSION}/dashboard.html#timelion-deprecation`, | |||
lens: `${ELASTIC_WEBSITE_URL}what-is/kibana-lens`, | |||
maps: `${ELASTIC_WEBSITE_URL}maps`, | |||
vega: `${ELASTIC_WEBSITE_URL}guide/en/kibana/${DOC_LINK_VERSION}/vega-graph.html`, |
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.
This URL doesn't exist any more. Try https://www.elastic.co/guide/en/kibana/7.10/vega-reference.html instead
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.
Yep, that's the point of (the first commit in) this PR. I wanted to make sure the docs build failed, before I replaced it with the correct URL.
Unfortunately, the build passed 😞 . Looks like I have some more work to do on the docs side.
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.
I should have read it more closely!
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.
It looks like, starting in 7.11, the page is now called just vega.html
.
@elasticmachine merge upstream |
@elasticmachine merge upstream |
This file's presence may have prevented checking the broken link I meant to add in this PR.
The build finally failed, and for the right reason:
Note the The failure is being ignored because kibana-bot doesn't consider these docs_link files to be "docs" changes. |
@elasticmachine run elasticsearch-ci/docs |
This is now failing for the right reason:
I'll make an update that fixes the link and references it from the correct place. @spalger I noticed the bot is still letting this pass, even though
|
@elasticmachine merge upstream |
@pgayvallet Can you let me know if the changes I made in 6cbb0c7 are the right way to pass the DocLinksService into where I can fetch the correct URL? Also, given how out of date this PR is, I'm not sure if the version labels are correct. Should this still go to 7.12.0, or 7.13.0 (and 7.12.1)? |
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.
@gtback implem looks good to me
Also, given how out of date this PR is, I'm not sure if the version labels are correct. Should this still go to 7.12.0, or 7.13.0 (and 7.12.1)?
7.13
for sure. Not sure if we need to backport to 7.12.1
?
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
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.
I haven't run this code, but it looks safe to me. This was a common pattern for a few months.
@@ -35,3 +35,5 @@ export const [getInjectedVars, setInjectedVars] = createGetterSetter<{ | |||
}>('InjectedVars'); | |||
|
|||
export const getEnableExternalUrls = () => getInjectedVars().enableExternalUrls; | |||
|
|||
export const [getDocLinks, setDocLinks] = createGetterSetter<DocLinksStart>('docLinks'); |
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.
Using createGetterSetter
is a bit of an outdated pattern, but I have no objection to following the Vega pattern until we actually refactor this.
cc @alexwizp for refactoring the Vega setup code
Friendly reminder: Looks like this PR hasn’t been backported yet. |
1 similar comment
Friendly reminder: Looks like this PR hasn’t been backported yet. |
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Summary
This link is currently wrong, but I'm testing to make sure it will get
caught if it's included in this file. A future commit will actually use
this variables in vega_help_menu.tsx, and update the link in
doc_links_service.ts
to be correct.Fixes #87367
Related: elastic/docs#1805
Checklist
Delete any items that are not applicable to this PR.
For maintainers