-
Notifications
You must be signed in to change notification settings - Fork 120
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: disable x filling for line and areas #833
fix: disable x filling for line and areas #833
Conversation
This fix removes the needs of filling the missing x values on line and area charts in the following case: non-stacked line or area chart, with continuous scale and no fit function. BREAKING CHANGE: On non-stacked line or area charts, with a continuous x scale and no fit function, the line between consecutive points, independently from the other data series, will be a continuous connecting line. fix elastic#825
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, path ordering story is still good.
|
||
function isXFillNotRequired(spec: BasicSeriesSpec, groupScaleType: ScaleType, isStacked: boolean) { | ||
const onlyNoFitAreaLine = (isAreaSeriesSpec(spec) || isLineSeriesSpec(spec)) && !spec.fit; | ||
const onlyContinuous = groupScaleType === ScaleType.Linear || groupScaleType === ScaleType.Time; |
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.
How does this affect Ordinal
scales? Do we not need to fit with Ordinal
scales?
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.
with Ordinal
scales it should work as before. the line, usually indicates a continuity on the data, using ordinal aka categorical scale the continuity is not the primary thing.
Let's stick with that for the moment, if there is the case to link discontinued points also on categorical scales we can fix that later, what do you think?
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.
Yeah that's fine. The fitting function logic does account for ordinal values, I was just wondering if this eliminated that functionality. Looks like it is still working as it was....
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.
Code changes LGTM. Verified fixes issue.
# [23.0.0](v22.0.0...v23.0.0) (2020-09-30) ### Bug Fixes * render continuous line/area between non-adjacent points ([#833](#833)) ([9f9892b](9f9892b)), closes [#825](#825) ### Features * debug state flag added to chart status ([#834](#834)) ([83919ff](83919ff)) * expose datum as part of GeometryValue ([#822](#822)) ([dcd7077](dcd7077)) ### BREAKING CHANGES * when rendering non-stacked line/area charts with a continuous x scale and no fit function, the line/area between non-consecutive data points will be rendered as a continuous line/area without adding an uncertain dashed line/ semi-transparent area that connects the two, non-adjacent, points.
🎉 This PR is included in version 23.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
# [23.0.0](elastic/elastic-charts@v22.0.0...v23.0.0) (2020-09-30) ### Bug Fixes * render continuous line/area between non-adjacent points ([opensearch-project#833](elastic/elastic-charts#833)) ([5222c40](elastic/elastic-charts@5222c40)), closes [opensearch-project#825](elastic/elastic-charts#825) ### Features * debug state flag added to chart status ([opensearch-project#834](elastic/elastic-charts#834)) ([f3aba25](elastic/elastic-charts@f3aba25)) * expose datum as part of GeometryValue ([opensearch-project#822](elastic/elastic-charts#822)) ([e582bd6](elastic/elastic-charts@e582bd6)) ### BREAKING CHANGES * when rendering non-stacked line/area charts with a continuous x scale and no fit function, the line/area between non-consecutive data points will be rendered as a continuous line/area without adding an uncertain dashed line/ semi-transparent area that connects the two, non-adjacent, points.
Summary
This fix removes the needs of filling the missing x values on line and area charts in the following
case: non-stacked line or area chart, with continuous scale and no fit function.
BREAKING CHANGE: On non-stacked line or area charts, with a continuous x scale and no fit function,
the line between consecutive points, independently from the other data series, will be a continuous
connecting line.
fix #825
Before the fix
After the fix
To Do
Checklist
Delete any items that are not applicable to this PR.
src/index.ts
(and stories only import from../src
except for test data & storybook)