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.

Sign up for GitHub

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

Jump to bottom

fix(partition): elimination of zero values #658

Merged
merged 5 commits into from
Apr 30, 2020
Merged

fix(partition): elimination of zero values #658

merged 5 commits into from
Apr 30, 2020

Conversation

monfera
Copy link
Contributor

@monfera monfera commented Apr 30, 2020 •
edited
Loading

Summary

Instead of eliminating negative values, zero values are also eliminated.
Fixes #642
Addresses last item of the minimum expectations checklist in #612

Checklist

Delete any items that are not applicable to this PR.

  • Any consumer-facing exports were added to src/index.ts (and stories only import from ../src except for test data & storybook)
  • This was checked for cross-browser compatibility, including a check against IE11
  • Proper documentation or storybook story was added for features that require explanation or tutorials
  • Unit tests were updated or added to match the most common scenarios

@monfera monfera self-assigned this Apr 30, 2020
@codecov-io
Copy link

codecov-io commented Apr 30, 2020 •
edited
Loading

Codecov Report

Merging #658 into master will not change coverage.
The diff coverage is 88.37%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #658   +/-   ##
=======================================
  Coverage   72.94%   72.94%           
=======================================
  Files         266      266           
  Lines        8618     8618           
  Branches     1695     1695           
=======================================
  Hits         6286     6286           
  Misses       2293     2293           
  Partials       39       39
Impacted Files Coverage Δ
...goal_chart/renderer/canvas/connected_component.tsx 34.42% <50.00%> (ø)
...ypes/partition_chart/renderer/canvas/partition.tsx 32.85% <50.00%> (ø)
...partition_chart/renderer/dom/highlighter_hover.tsx 44.44% <50.00%> (ø)
...artition_chart/renderer/dom/highlighter_legend.tsx 44.44% <50.00%> (ø)
src/chart_types/goal_chart/state/chart_state.tsx 58.33% <66.66%> (ø)
...tion_chart/layout/viewmodel/hierarchy_of_arrays.ts 88.88% <100.00%> (ø)
.../chart_types/partition_chart/state/chart_state.tsx 74.00% <100.00%> (ø)
.../chart_types/xy_chart/renderer/canvas/xy_chart.tsx 86.30% <100.00%> (ø)
...ypes/xy_chart/renderer/dom/annotation_tooltips.tsx 52.13% <100.00%> (ø)
src/chart_types/xy_chart/renderer/dom/brush.tsx 80.39% <100.00%> (ø)
... and 18 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 89eaf0d...019323a. Read the comment docs.

@monfera
Copy link
Contributor Author

monfera commented Apr 30, 2020

Jenkins retest this

@monfera
Copy link
Contributor Author

monfera commented Apr 30, 2020

Jenkins retest this please

markov00
markov00 approved these changes Apr 30, 2020
View reviewed changes
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.

Code LGTM. I've tested locally and the mentioned issue is fixed.

src/chart_types/partition_chart/layout/viewmodel/hierarchy_of_arrays.test.ts Outdated

const rawFacts = [
{ sitc1: '7', exportVal: 0 },
{ sitc1: '3', exportVal: 1929578418424 },
Copy link
Member

Choose a reason for hiding this comment

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

nit: you could also include a test against negative values

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added it with 019323a

@monfera monfera merged commit 9ee67dc into elastic:master Apr 30, 2020
@monfera monfera deleted the zero-treemap branch April 30, 2020 22:59
@monfera monfera mentioned this pull request May 1, 2020
Closed
markov00 pushed a commit that referenced this pull request May 4, 2020
@semantic-release-bot
chore(release): 19.1.2 [skip ci]
16d9974 
## [19.1.2](v19.1.1...v19.1.2) (2020-05-04)

### Bug Fixes

* **partition:** elimination of zero values ([#658](#658)) ([9ee67dc](9ee67dc)), closes [#642](#642)
@markov00
Copy link
Member

markov00 commented May 4, 2020

🎉 This PR is included in version 19.1.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label May 4, 2020
@markov00 markov00 mentioned this pull request May 4, 2020
Merged
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
@semantic-release-bot
chore(release): 19.1.2 [skip ci]
daa21f2 
## [19.1.2](elastic/elastic-charts@v19.1.1...v19.1.2) (2020-05-04)

### Bug Fixes

* **partition:** elimination of zero values ([opensearch-project#658](elastic/elastic-charts#658)) ([6e7b3dd](elastic/elastic-charts@6e7b3dd)), closes [opensearch-project#642](elastic/elastic-charts#642)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markov00 markov00 markov00 approved these changes

Assignees

@monfera monfera

Labels
released Issue released publicly
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Treemap in infinite loop when valueAccessor returns zero
3 participants
@monfera @codecov-io @markov00