Skip to content
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: empty domain error for ordinal x scale #536

Merged
merged 2 commits into from
Feb 5, 2020

Conversation

nickofthyme
Copy link
Collaborator

Summary

Fixes issue related to empty domain.

If the data is an empty array and the xScaleType is Ordinal the scaleBand from d3-scale will run

new Array(-1)

And throw a RangeError. This is a quick fix, but ideally, we revisit this to avoid unnecessarily computing the domain if the data is empty for all specs.

Checklist

  • Unit tests were updated or added to match the most common scenarios
  • Each commit follows the convention

@nickofthyme nickofthyme added the bug Something isn't working label Feb 5, 2020
@nickofthyme nickofthyme requested a review from markov00 February 5, 2020 17:41
@@ -93,6 +93,11 @@ export function mergeXDomain(
minInterval = customMinInterval || computedMinInterval;
}

// TODO: Check for empty data before computing domain
if (seriesXComputedDomains.length === 0) {
seriesXComputedDomains = [0, 0];
Copy link
Member

Choose a reason for hiding this comment

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

This should be just one single value [0] because we are in ordinal domain and we don't want duplicated domain values

Copy link
Member

Choose a reason for hiding this comment

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

it's also better to move that inside the computeOrdinalDataDomain of domains.ts

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@codecov-io
Copy link

codecov-io commented Feb 5, 2020

Codecov Report

Merging #536 into master will increase coverage by 0.24%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #536      +/-   ##
==========================================
+ Coverage   75.56%   75.81%   +0.24%     
==========================================
  Files         181      193      +12     
  Lines        5689     5846     +157     
  Branches     1103     1117      +14     
==========================================
+ Hits         4299     4432     +133     
- Misses       1374     1397      +23     
- Partials       16       17       +1
Impacted Files Coverage Δ
src/utils/domain.ts 97.43% <100%> (+0.13%) ⬆️
src/mocks/series/series.ts 81.96% <0%> (ø)
src/mocks/specs/specs.ts 77.27% <0%> (ø)
src/mocks/scale/index.ts 100% <0%> (ø)
src/mocks/index.ts 100% <0%> (ø)
src/mocks/hierarchical/palettes.ts 100% <0%> (ø)
src/mocks/series/index.ts 100% <0%> (ø)
src/mocks/series/seriesIdentifiers.ts 93.33% <0%> (ø)
src/mocks/scale/scale.ts 87.5% <0%> (ø)
src/mocks/utils.ts 100% <0%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cc561a4...c41c0ef. Read the comment docs.

Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

LGTM

@nickofthyme nickofthyme merged commit ce4e84f into elastic:master Feb 5, 2020
@nickofthyme nickofthyme deleted the fix/empty-domain-error branch February 5, 2020 18:39
markov00 pushed a commit that referenced this pull request Feb 5, 2020
## [17.0.2](v17.0.1...v17.0.2) (2020-02-05)

### Bug Fixes

* empty domain error for ordinal x scale ([#536](#536)) ([ce4e84f](ce4e84f))
@markov00
Copy link
Member

markov00 commented Feb 5, 2020

🎉 This PR is included in version 17.0.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label Feb 5, 2020
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working released Issue released publicly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants