fix(area_charts): correctly represent baseline with negative data points #896
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit fix the rendering issue when using negative values in area charts. The correct approach is to draw the area with a zero baseline (or dummy one baseline for log y scale). This correctly represent the sense of area for a single data point. fix elastic#893
Codecov Report
@@ Coverage Diff @@
## master #896 +/- ##
==========================================
- Coverage 69.65% 69.65% -0.01%
==========================================
Files 335 340 +5
Lines 10905 10950 +45
Branches 2269 2276 +7
==========================================
+ Hits 7596 7627 +31
- Misses 3295 3309 +14
Partials 14 14
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
markov00
force-pushed
the
2020_11_06-fix_negative_areas
branch
from
41f2a5e to
8cef28f
Compare
markov00
force-pushed
the
2020_11_06-fix_negative_areas
branch
from
aa63501 to
eca4136
Compare
nickofthyme
approved these changes
Nov 10, 2020
There was a problem hiding this comment.
Code changes LGTM. Tested locally and everything functions as expected.
Why does the band disappear when using log scale? 👇
Also, I noticed that for positive and negative values the log scale just shows the positive values. Is there a way to logically/correctly show positive and negative values when using log scale? Like the symmetric log scale, @monfera was talking about?
Comment on lines
714
to
717
| if (isLogScale) { | ||
| return yScale.scaleOrThrow(1); | ||
| } | ||
| return yScale.scaleOrThrow(0); |
There was a problem hiding this comment.
Would these just return a consistent pixel value like 0 rather than scaling the values?
There was a problem hiding this comment.
Also, the 1 should be LOG_MIN_ABS_DOMAIN right?
There was a problem hiding this comment.
already solved on eca4136
rshen91
approved these changes
Nov 10, 2020
This happens because the lowest point is below the threshold of 1 (currently 0.9). In this case, the band is not defined and is removed from the path |
This was referenced Nov 12, 2020
markov00
pushed a commit
that referenced
this pull request
Nov 24, 2020
# [24.1.0](v24.0.0...v24.1.0) (2020-11-24) ### Bug Fixes * **area_charts:** correctly represent baseline with negative data points ([#896](#896)) ([d1243f1](d1243f1)) * **legend:** legend sizes with ordinal data ([#867](#867)) ([7559e0d](7559e0d)), closes [#811](#811) * render orphan data points on lines and areas ([#900](#900)) ([0be282b](0be282b)), closes [#783](#783) * specs swaps correctly reflected in state ([#901](#901)) ([7fba882](7fba882)) ### Features * **legend:** allow legend text to be copyable ([#877](#877)) ([9cd3459](9cd3459)), closes [#710](#710) * allow clearing series colors from memory ([#899](#899)) ([ab1af38](ab1af38)) * merge series domain with the domain of another group ([#912](#912)) ([325b013](325b013)) * small multiples for XY charts (alpha) ([#793](#793)) ([d288208](d288208)), closes [#500](#500) [#500](#500)
|
🎉 This PR is included in version 24.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
2 tasks
markov00
added a commit
that referenced
this pull request
Jun 3, 2021
Domain bounds should be used in the sense of zooming in/out with a viewport on the chart. A regression of this behavior slipped through due to a [PR](#896) causing data to be filtered out when applying a custom domain, removing data points from areas and lines. The right behavior is now restored. fix #1129
AMoo-Miki
pushed a commit
to AMoo-Miki/OpenSearch-Dashboards
that referenced
this pull request
Feb 10, 2022
# [24.1.0](elastic/elastic-charts@v24.0.0...v24.1.0) (2020-11-24) ### Bug Fixes * **area_charts:** correctly represent baseline with negative data points ([opensearch-project#896](elastic/elastic-charts#896)) ([b622fda](elastic/elastic-charts@b622fda)) * **legend:** legend sizes with ordinal data ([opensearch-project#867](elastic/elastic-charts#867)) ([74bcbad](elastic/elastic-charts@74bcbad)), closes [opensearch-project#811](elastic/elastic-charts#811) * render orphan data points on lines and areas ([opensearch-project#900](elastic/elastic-charts#900)) ([3e2c739](elastic/elastic-charts@3e2c739)), closes [opensearch-project#783](elastic/elastic-charts#783) * specs swaps correctly reflected in state ([opensearch-project#901](elastic/elastic-charts#901)) ([a94347f](elastic/elastic-charts@a94347f)) ### Features * **legend:** allow legend text to be copyable ([opensearch-project#877](elastic/elastic-charts#877)) ([21a96d3](elastic/elastic-charts@21a96d3)), closes [opensearch-project#710](elastic/elastic-charts#710) * allow clearing series colors from memory ([opensearch-project#899](elastic/elastic-charts#899)) ([e97f4ab](elastic/elastic-charts@e97f4ab)) * merge series domain with the domain of another group ([opensearch-project#912](elastic/elastic-charts#912)) ([716ad5a](elastic/elastic-charts@716ad5a)) * small multiples for XY charts (alpha) ([opensearch-project#793](elastic/elastic-charts#793)) ([3b88e1c](elastic/elastic-charts@3b88e1c)), closes [opensearch-project#500](elastic/elastic-charts#500) [opensearch-project#500](elastic/elastic-charts#500)
AMoo-Miki
pushed a commit
to AMoo-Miki/OpenSearch-Dashboards
that referenced
this pull request
Feb 10, 2022
…#1181) Domain bounds should be used in the sense of zooming in/out with a viewport on the chart. A regression of this behavior slipped through due to a [PR](elastic/elastic-charts#896) causing data to be filtered out when applying a custom domain, removing data points from areas and lines. The right behavior is now restored. fix opensearch-project#1129
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes the rendering issue when using negative values in area charts. The correct approach
is to draw the area with a zero baseline (or dummy one baseline for log y scale). This correctly
represent the sense of area for a single data point.
fix #893
Note for reviewers
It's easier to check the changes if you review them by commit:
Another thing to notice: the change related to the log scale also changed the way we render areas in the [-1,1] range. I've adopted a different strategy that removes areas/points from showing in that range (possibly adjustable via theme/configuration in a future PR). This adjustment will require the implementation of the #63 request.
Checklist