-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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] Fix DFA feature importance popover empty #91061
Conversation
Pinging @elastic/ml-ui (:ml) |
importance: importance[index], | ||
}) | ||
: fi.classes, | ||
importance: Array.isArray(fi.importance) ? fi.importance[0] : fi.importance, |
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.
From the type, it looks like 'importance' is a number. If it can also be an array of numbers it might be worth updating the type.
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.
Left a small comment but code LGTM ⚡
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.
Tested and LGTM
// Make sure the charts tooltip in popover | ||
// have higher zIndex than Eui popover cells | ||
[id^='echTooltipPortal'] { | ||
z-index: 10000; |
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 works for me only when adding !important
as otherwise it gets overridden by the inline style.
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.
Thanks Pete, this is now updated here 3f76bf8
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.
Latest edits LGTM
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.
Left one small SCSS comment, but LGTM otherwise. Approving now to avoid holding you up.
// Make sure the charts tooltip in popover | ||
// have higher zIndex than Eui popover cells | ||
[id^='echTooltipPortal'] { | ||
z-index: 10000 !important; |
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.
Assuming this achieves the same result, consider changing to an EUI z-index variable.
z-index: 10000 !important; | |
z-index: $euiZLevel9 !important; |
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.
Updated here 3f25370
💚 Build SucceededMetrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
Co-authored-by: Quynh Nguyen <[email protected]>
Summary
This PR fixes #90603 which was introduced due to recent
fields
API changes. It also unskip related functional tests that were previously skipped here #90526Classification
Regression
Review Hints:
expandable_section.scss
to fix the z-index of the Elastic charts tooltip (100) much smaller than the Eui popover z-index (8001)Checklist