Skip to content

[Lens] Thresholds: auto fit thresholds into vertical axis#113238

Merged
dej611 merged 8 commits intoelastic:masterfrom
dej611:fix/112795
Oct 6, 2021
Merged

[Lens] Thresholds: auto fit thresholds into vertical axis#113238
dej611 merged 8 commits intoelastic:masterfrom
dej611:fix/112795

Conversation

@dej611
Copy link
Contributor

@dej611 dej611 commented Sep 28, 2021

Summary

Fixes #112795

This PR includes thresholds into the vertical axis computation to fit them together with the data.

Main points:

  • when in full extends mode the thresholds should be taken into account.
  • when in data bounds mode (line chart) thresholds should not be taken into account
  • when in custom mode it has priority over data and thresholds

Note 1:
Does it make sense add some padding to the extends in cases as the ones below?

Screenshot 2021-09-28 at 11 34 36

Screenshot 2021-09-28 at 11 34 56

Screenshot 2021-09-28 at 11 35 50

Screenshot 2021-09-28 at 11 52 00

Note 2: the auto extends does not apply for the horizontal axis:
Screenshot 2021-09-28 at 11 50 20

Checklist

Delete any items that are not applicable to this PR.

@dej611 dej611 added Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:Lens auto-backport Deprecated - use backport:version if exact versions are needed v7.16.0 labels Sep 28, 2021
@dej611 dej611 marked this pull request as ready for review September 28, 2021 16:03
@dej611 dej611 requested a review from a team as a code owner September 28, 2021 16:03
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-vis-editors (Team:VisEditors)

@dej611
Copy link
Contributor Author

dej611 commented Sep 29, 2021

@elasticmachine merge upstream

@dej611
Copy link
Contributor Author

dej611 commented Sep 30, 2021

@elasticmachine merge upstream

@dej611
Copy link
Contributor Author

dej611 commented Oct 4, 2021

@elasticmachine merge upstream

@stratoula
Copy link
Contributor

@elasticmachine merge upstream

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

This looks fine! Code LGTM, I tested it locally and works as it is described in the issue.
About the padding I don't have strong opinion. It would make sense to me but then we should decide the padding value.
@ghudgins how do you feel about it?

max = extent.upperBound;
}
} else {
const axisHasTreshold = thresholdLayers.some(({ yConfig }) =>
Copy link
Contributor

@stratoula stratoula Oct 5, 2021

Choose a reason for hiding this comment

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

nit: typo, axisHasThreshold :D

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
lens 1.0MB 1.0MB +502.0B

History

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

@dej611 dej611 merged commit 3fe1eab into elastic:master Oct 6, 2021
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Oct 6, 2021
…3238)

* ✨ Make threshold fit into view automatically

* 🐛 do not compute axis threshold extends if no threshold is present

* ✅ One more fix for 0-based extends and tests

* 📝 fix typo

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Oct 6, 2021
…114104)

* ✨ Make threshold fit into view automatically

* 🐛 do not compute axis threshold extends if no threshold is present

* ✅ One more fix for 0-based extends and tests

* 📝 fix typo

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Marco Liberati <dej611@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-backport Deprecated - use backport:version if exact versions are needed Feature:Lens release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v7.16.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Lens] Thresholds: chart bounds should be automatically extended to include threshold lines

4 participants