-
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] Fix Single Metric Viewer annotation tooltip hard to trigger #98233
Conversation
2ffc818
to
eba2464
Compare
Pinging @elastic/ml-ui (:ml) |
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
@elasticmachine merge upstream |
annotationUpdatesService.setValue(d); | ||
}); | ||
.on('mouseover', onAnnotationMouseOver) | ||
.on('mouseout', onAnnotationMouseOut) |
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.
Can't hideFocusChartTooltip
be passed here directly since it takes no arguments? Does it need to be wrapped in the onAnnotationMouseOut
arrow function?
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 for catching that. Updated here dc9798b
(#98233).
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 otherwise LGTM ⚡
x-pack/plugins/ml/public/application/timeseriesexplorer/_timeseriesexplorer_annotations.scss
Outdated
Show resolved
Hide resolved
@@ -132,6 +132,19 @@ export function renderAnnotations( | |||
const levelHeight = ANNOTATION_LEVEL_HEIGHT; | |||
const levels = getAnnotationLevels(focusAnnotationData); | |||
|
|||
const onAnnotationMouseOver = function (this: object, d: Annotation) { |
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.
Is there any type of 'mouseover'
cotext (this) available?
...c/application/timeseriesexplorer/components/timeseries_chart/timeseries_chart_annotations.ts
Outdated
Show resolved
Hide resolved
...c/application/timeseriesexplorer/components/timeseries_chart/timeseries_chart_annotations.ts
Show resolved
Hide resolved
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.
If I'm following correctly, I believe there is a fix for the focus ring coming on the EUI side (cc: @cchaos ). See the discussion and related flyout PR.
Presuming this is correct, it might be best to wait on the EUI fix. At minimum, linking to the discussion/PR in a comment and coming back to remove this would be recommended.
.mlAnnotationFlyout { | ||
&:focus { | ||
outline: none; | ||
} | ||
} |
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.
As @ryankeairns said, this will be fixed via EUI before 7.14. So I'd recommend not doing any overrides.
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 @ryankeairns and @cchaos for the context 🙏 Good to know the issue will be fixed in 7.13. I have removed the css overrides here baea0c3
(#98233)
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 for the change! Also, with the CSS removed, you might be able to remove the className
.
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: cc @qn895 |
…stic#98233) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
) (#98804) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Quynh Nguyen <43350163+qn895@users.noreply.github.com>
…stic#98233) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
) (#99352) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Summary
This PR is part of #69538. Fixes include:
Make annotation marker labels on the Single Metric Viewer focus chart hover and clickable. Previously, if the annotation has a small duration, it would be extremely hard for user to be able to hover over the straight line to read the tooltip information.
Before
After
Fix a styling issue with EUI Amsterdam theme which cause the flyout to have a black border when the page is first loaded and an svg marker is the first item used to trigger the
Edit annotation
flyout. See related EUI issue.Before
After