-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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] Migrate ml-info-icon to EUI/React #19003
Conversation
Pinging @elastic/ml-ui |
💚 Build Succeeded |
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.
LGTM. Several steps later it will be nice if we can just replace this with a wrapper around EuiIconTip
to use the text in the tooltips.json
file.
💚 Build Succeeded |
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.
LGTM
Just one comment about end of file new lines
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import './tooltip_directive'; |
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.
new line missing here.
not sure if this is a bad thing as there is no lint rule for it. but github flags it up.
"files.insertFinalNewline": true
in vscode will do this automatically.
The same for x-pack/plugins/ml/public/components/tooltip/tooltip_directive.js
💔 Build Failed |
jenkins test this |
💚 Build Succeeded |
This replaces the angular based tooltip used in the ml-info-icon directive with one based on React/EuiTooltip. It supports transclusion and rendering of angular template snippets inside the tooltip. Because the DOM structure of the EuiTooltips and the angular/bootstrap tooltips is quite different there is a bit more code involved to achieve this, but the result is that for this we don't have to change the markup in the original angular templates where ml-info-icon is used.
This replaces the angular based tooltip used in the ml-info-icon directive with one based on React/EuiTooltip. It supports transclusion and rendering of angular template snippets inside the tooltip. Because the DOM structure of the EuiTooltips and the angular/bootstrap tooltips is quite different there is a bit more code involved to achieve this, but the result is that for this we don't have to change the markup in the original angular templates where ml-info-icon is used.
💚 Build Succeeded |
This replaces the angular based tooltip used in the
ml-info-icon
directive with one based on React/EuiTooltip. It supports transclusion and rendering of angular template snippets inside the tooltip. Because the DOM structure of the EuiTooltips and the angular/bootstrap tooltips is quite different there is a bit more code involved to achieve this, but the result is that for this we don't have to change the markup in the original angular templates whereml-info-icon
is used.This doesn't use
ngReact
to have more detailed control on how to work with the transclusion of angular template snippets as well as skip any two-way-binding which isn't necessary for these type of tooltips.Follow-up PRs will use this as a basis to replace other angular based tooltips.
Part of #18374.