Threshold line on bar/line/area charts #42632
Conversation
|
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
timroes
added
Feature:XYAxis
|
Pinging @elastic/kibana-app |
|
Jenkins, test this |
|
Hi Filip, thanks a lot for your contribution. We'll try to review and test this as soon as possible to get it merged as quickly as possible. Could you please make sure this is up to date with @markov00 do we already have a threshold line feature in Elastic Charts? If not I think we should open an issue for that, so we can move that feature over with us :-) Cheers, |
|
Also some heads up, we currently have another PR open that converts the options panel for point series charts to React (#42828). If that progresses faster than this PR, we would need to ask you to rewrite the editor part in React, but can't right now tell you which one will be faster, sorry. |
💔 Build Failed |
|
Those test failures seem related to this PR. |
@timroes I believe I fixed the bug causing those tests to fail. You can kick off the build again. |
|
Jenkins, test this |
💚 Build Succeeded |
|
We can also color bars above a specific threshold if you want :) |
|
Testing this PR. It seems that we currently show the line on the very top or bottom of the chart if the value the line should be is outside the chart:
This is in my opinion a bit confusing, since this could lead the user to thinking the line would (in that example) sit at 16 (or just above the actual value series) while it actually is at 100. Having negative threshold lines might look even more confusing since it would just sit at 0 in this chart. I am wondering if we should hide the line altogether in case it's outside the chart? @markov00 @monfera any opinions? Also I would let the user give the line a specific color. Currently we just get one of the series colors, but that's kind of very random. So if you have a split series with 5 different lines it'll just get the color of one of the lines, even though there is no connection between the threshold line and that specific series. I think that would be better if the user just decides on the color manually. Since (as mentioned above) we'll very shortly merge the PR that converts that editor to React, I fear you'll need to rewrite that editor in React, and thus I would not implement the color picker before we have merged the other PR, so you're not unnecessary doing some color picker stuff now in Angular, while you anyway need to rewrite it then in React (where we can actually just use the EUI color picker). |
|
@FilipRy We now merged the PR with React refactoring and this PR would need to be needed to brought up in sync with master. |
|
Jenkins, test this |
💔 Build Failed |
|
Jenkins, test this - git failure in CI |
💔 Build Failed |
|
Jenkins, test this - another try |
💔 Build Failed |
There was a problem hiding this comment.
Tested on Chrome Linux. Works as I would expect it to work in a wide range of tested scenarios. Multiple y-axis will cause multiple threshold lines, one for each y-axis, which is what I would assume to happen (even though in that case not that useful, but that's up to the user to decide). Checked the code, and code LGTM. Waiting for CI to work for getting this green.
|
Jenkins, test this |
💔 Build Failed |
|
Jenkins, test this |
💔 Build Failed |
|
jenkins test this please |
|
Jenkins, test this |
1 similar comment
|
Jenkins, test this |
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
|
Jenkins, test this |
timroes
changed the title
💔 Build Failed |
|
Jenkins, test this - I fixed a couple of remaining TS issues |
💚 Build Succeeded |
* first steps towards threshold line (histogram) * threshold line added for all point_series charts * added settings for threshold line * last fixes * fixed typo * default values for thresholdLineOptions * resolving conflicts * threshold line not displayed when out of the canvas * linting * added color picker for threshold line * fixed assigning of a static color and i18 select options * changing default color and lintings * Fix remaining TS issues
* first steps towards threshold line (histogram) * threshold line added for all point_series charts * added settings for threshold line * last fixes * fixed typo * default values for thresholdLineOptions * resolving conflicts * threshold line not displayed when out of the canvas * linting * added color picker for threshold line * fixed assigning of a static color and i18 select options * changing default color and lintings * Fix remaining TS issues
|
Filip, thanks you a lot for providing this change. I merged and backported it now, so it'll be released in Kibana in 7.4. Thanks for your effort and I hope we see further contributions from you to Kibana 🎉 |
Summary
Closes #5037.
User can define a threshold which is visualized as a line on the chart. The width and style [full line, dashed line, dash-dotted line] of the line can be set by the user as well.
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.For maintainers