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

Improve and cleanup chrome helpMenu links #82300

Merged
merged 19 commits into from
Dec 11, 2020

Conversation

pgayvallet
Copy link
Contributor

@pgayvallet pgayvallet commented Nov 2, 2020

Summary

Fix #82158

  • Cleanup the HeaderHelpMenu component and associated types
  • Use application.navigateToUrl under the hood for custom internal links to avoid a full page refresh

Checklist

@pgayvallet pgayvallet added chore release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.11.0 labels Nov 2, 2020
Comment on lines +45 to +48
export type ChromeHelpExtensionLinkBase = Pick<
EuiButtonEmptyProps,
'iconType' | 'target' | 'rel' | 'data-test-subj'
>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Restrict the links base type (which was EuiButtonEmptyProps previously) to the only properties that make sense for the helpMenu links.

Comment on lines -99 to +113
export type ChromeHelpExtensionMenuLink = ExclusiveUnion<
ChromeHelpExtensionMenuGitHubLink,
ExclusiveUnion<
ChromeHelpExtensionMenuDiscussLink,
ExclusiveUnion<ChromeHelpExtensionMenuDocumentationLink, ChromeHelpExtensionMenuCustomLink>
>
>;
export type ChromeHelpExtensionMenuLink =
| ChromeHelpExtensionMenuGitHubLink
| ChromeHelpExtensionMenuDiscussLink
| ChromeHelpExtensionMenuDocumentationLink
| ChromeHelpExtensionMenuCustomLink;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing the ExclusiveUnion adds a (little) more boilerplate in the switch type logic in renderCustomContent, but it's way more readable, and the tsdoc is greatly improved.

}

export const HeaderHelpMenu = injectI18n(HeaderHelpMenuUI);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The component was using a mix of i18n and intl. Cleaned that

Comment on lines +323 to +330
case 'custom': {
const { linkType, content: text, href, ...rest } = link;
return createCustomLink(index, text, addSpacer, {
href,
onClick: this.createOnClickHandler(href, navigateToUrl),
...rest,
});
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only functional change of the PR: we now add a onClick handler to custom links to perform SPA navigation when the link is internal.

@pgayvallet pgayvallet marked this pull request as ready for review November 2, 2020 17:39
@pgayvallet pgayvallet requested review from a team as code owners November 2, 2020 17:39
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform (Team:Platform)

Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

Updates to app arch generated docs LGTM

@botelastic botelastic bot added the Feature:Embedding Embedding content via iFrame label Nov 3, 2020
@pgayvallet pgayvallet removed the Feature:Embedding Embedding content via iFrame label Nov 5, 2020
@botelastic botelastic bot added the Feature:Embedding Embedding content via iFrame label Nov 5, 2020
@pgayvallet
Copy link
Contributor Author

@elastic/kibana-operations The PR was building until fb9376e that reintegrated master into it. I looked quite a bit, but I just don't understand what the issue is or how this PR could cause such failure

Error from https://kibana-ci.elastic.co/job/elastic+kibana+pipeline-pull-request/87975/execution/node/106/log/ is

 ERROR Error: Command failed with exit code 2: /dev/shm/workspace/kibana/node_modules/typescript/bin/tsc -b tsconfig.refs.json --pretty
14:50:43              packages/kbn-test/target/types/jest/utils/enzyme_helpers.d.ts:29:11 - error TS2503: Cannot find namespace 'ReactIntl'.
14:50:43  
14:50:43              29     intl: ReactIntl.InjectedIntl;
14:50:43                           ~~~~~~~~~
14:50:43  
14:50:43  
14:50:43              Found 1 error.

Could someone take a quick look?

@spalger
Copy link
Contributor

spalger commented Nov 18, 2020

I have no idea what's going on here, but maybe @restrry has some idea why this is happening when building refs. It seems that the ReactIntl namespace/global is found when VSCode loads up the file, but not during bootstrap... I wonder if this has to do with the removal of the "react" types?

@pgayvallet
Copy link
Contributor Author

I wonder if this has to do with the removal of the "react" types?

I also thought that, but adding the react imports back do not fix that thing.

@pgayvallet
Copy link
Contributor Author

#83803 will address the issue and unblock this PR.

@pgayvallet
Copy link
Contributor Author

retest

Copy link
Contributor

@TinaHeiligers TinaHeiligers left a comment

Choose a reason for hiding this comment

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

Code review and locally tested the work.

Functionally, the PR introduces a small change that works as intended. On a larger scale, the PR makes big improvements and cleans up a few areas too.

LGTM!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Distributable file count

id before after diff
default 47124 47884 +760

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
core 552.7KB 553.2KB +433.0B

History

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

@pgayvallet pgayvallet merged commit a4caffa into elastic:master Dec 11, 2020
pgayvallet added a commit to pgayvallet/kibana that referenced this pull request Dec 11, 2020
* Improve and cleanup chrome helpMenu links

* update doc due to merge

* remove dev dependencies from test plugin

* update generated doc after merge

* update generated doc

* generated doc

* generated doc
pgayvallet added a commit that referenced this pull request Dec 11, 2020
* Improve and cleanup chrome helpMenu links

* update doc due to merge

* remove dev dependencies from test plugin

* update generated doc after merge

* update generated doc

* generated doc

* generated doc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Feature:Embedding Embedding content via iFrame release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.11.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Internal links registered with core.chrome.setHelpExtension cause full page reload
6 participants