-
Notifications
You must be signed in to change notification settings - Fork 122
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
fix: x-scale for linear band charts #384
Conversation
Add respective local directory to tsconfig (i.e. ./.stroybook and ./.playground)
Codecov Report
@@ Coverage Diff @@
## master #384 +/- ##
==========================================
+ Coverage 98.25% 98.26% +0.01%
==========================================
Files 38 38
Lines 2801 2818 +17
Branches 661 670 +9
==========================================
+ Hits 2752 2769 +17
Misses 44 44
Partials 5 5
Continue to review full report at Codecov.
|
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.
LGTM. Tested locally
One side note: IIRC you tried to add .nice()
on the scale configuration. Can you please write down what didn't work?
@@ -182,13 +199,13 @@ export class ScaleContinuous implements Scale { | |||
return currentDateTime.minus({ minutes: currentOffset }).toMillis(); | |||
}); | |||
} else { | |||
if (this.minInterval > 0) { | |||
if (minInterval > 0 && bandwidth > 0) { |
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.
Would you mind adding a comment before this line specifying that:
- this case happens only on the xScale (minInterval is > 0 only in that case) and when we want to show bars (bandwidth > 0)
- the reason is: we want to avoid displaying inner ticks between bars in a bar chart when using linear x scale: this can be misinterpreted because the bar width can be interpreted as an interval along that scale
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.
Yes the I think the issue could be a simple fix but I couldn't get it. I opened a new issue to fix this in the future 👉 #397 |
## [13.0.1](v13.0.0...v13.0.1) (2019-09-27) ### Bug Fixes * x-scale for linear band charts ([#384](#384)) ([daa3b55](daa3b55))
🎉 This PR is included in version 13.0.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
## [13.0.1](elastic/elastic-charts@v13.0.0...v13.0.1) (2019-09-27) ### Bug Fixes * x-scale for linear band charts ([opensearch-project#384](elastic/elastic-charts#384)) ([16e216d](elastic/elastic-charts@16e216d))
Summary
Add check for
bandwidth
to avoid computingminInterval
for linear band charts.fixes #364
Checklist