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

[ML] Add annotation markers to time series brush area to indicate annotations exist outside of selected range #81490

Merged

Conversation

qn895
Copy link
Member

@qn895 qn895 commented Oct 22, 2020

Summary

This PR fixes #54025 by adding markers for annotations in the time series brush area to show and that annotations exist outside of the selected, visible range

Screen Shot 2020-10-22 at 09 37 17

Screen Shot 2020-10-28 at 15 55 33

@qn895
Copy link
Member Author

qn895 commented Oct 27, 2020

@elasticmachine merge upstream

@qn895 qn895 marked this pull request as ready for review October 27, 2020 13:29
@qn895 qn895 requested review from a team as code owners October 27, 2020 13:29
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

I like the way the annotations are shown for the full chart area! Left a couple of comments about refining the query used to load the annotations.

Copy link
Contributor

@darnautov darnautov left a comment

Choose a reason for hiding this comment

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

Latest edits LGTM! 🚀

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

Tested latest changes and LGTM.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

id before after diff
ml 1237 1238 +1

async chunks size

id before after diff
ml 6.6MB 6.6MB +24.6KB

History

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

// context annotation marker
.mlContextAnnotationRect {
stroke: $euiColorFullShade;
stroke-width: $mlAnnotationBorderWidth;
Copy link
Contributor

Choose a reason for hiding this comment

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

For what it's worth, there are 1px $euiBorderWidthThin and 2px $euiBorderWidthThick variables, if you're interested in using them here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for looking it over 🙏 I think I'll keep mlAnnotationBorderWidth here since it's used for other parts as well but good to know we do have those variables available.

@qn895 qn895 merged commit 8988798 into elastic:master Nov 4, 2020
@qn895 qn895 deleted the ml-add-annotation-markers-in-context-brush-area branch November 4, 2020 17:02
qn895 added a commit to qn895/kibana that referenced this pull request Nov 4, 2020
…otations exist outside of selected range (elastic#81490)

Co-authored-by: Kibana Machine <[email protected]>
qn895 added a commit that referenced this pull request Nov 4, 2020
…te annotations exist outside of selected range (#81490) (#82632)

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Kibana Machine <[email protected]>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Nov 5, 2020
* master: (127 commits)
  [ILM] Fix breadcrumbs (elastic#82594)
  [UX]Swap env filter with percentile (elastic#82246)
  Add platform's missing READMEs (elastic#82268)
  [Discover] Adding uiMetric to track Visualize link click (elastic#82344)
  [Search] Add used index pattern name to the search agg error field (elastic#82604)
  improve client-side SO client get pooling (elastic#82603)
  [Security Solution] Unskips Overview tests (elastic#82459)
  Embeddables/migrations (elastic#82296)
  [Enterprise Search] Refactor product server route registrations to their own files/folders (elastic#82663)
  Moving reinstall function outside of promise.all (elastic#82672)
  Load choropleth layer correctly (elastic#82628)
  Master  backport elastic#81233 (elastic#82642)
  [Fleet] Allow snake cased Kibana assets (elastic#77515)
  Reduce saved objects authorization checks (elastic#82204)
  [data.search] Add request handler context and asScoped pattern (elastic#80775)
  [ML] Fixes formatting of fields in index data visualizer (elastic#82593)
  Usage collector readme (elastic#82548)
  [Lens] Visualization validation and better error messages (elastic#81439)
  [ML] Add annotation markers to time series brush area to indicate annotations exist outside of selected range (elastic#81490)
  chore(NA): install microdnf in UBI docker build only (elastic#82611)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ML] Single metric viewer should indicate existence of annotations that are beyond the selected time region
6 participants