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(ticks): fill in additional ticks for histogram #251

Merged
merged 5 commits into from
Jul 5, 2019

Conversation

emmacunningham
Copy link
Contributor

Summary

This PR introduces a fix for the issue where, in histogram mode, the intermediary ticks for a time scale were not being rendered.

The issue stemmed from how the ticks were being extended in histogram mode. Previously, we were just extending the ticks by 1 tick adding the scale's minInterval to the last computed tick. However, in cases where the interval between ticks was smaller than the scale's minInterval, this would mean that while the last tick would be correct, there would be missing intermediary ticks between the last computed tick and the final added tick. Now, we computed the distance between the ticks and add additional ticks with that interval until reaching the extended histogram domain end.

Before fix (note the missing ticks for the last bar):
custom_min_interval

After fix:
intermediary_ticks

And additional test has been added to simulate the situation visible in the Storybook example and have also cleaned up the tests a bit to be easier to follow.

Checklist

Use strikethroughs to remove checklist items you don't feel are 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)
- [ ] Proper documentation or storybook story was added for features that require explanation or tutorials

@emmacunningham emmacunningham added :axis Axis related issue bug Something isn't working labels Jul 1, 2019
@codecov-io
Copy link

codecov-io commented Jul 1, 2019

Codecov Report

Merging #251 into master will increase coverage by 0.17%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #251      +/-   ##
==========================================
+ Coverage   97.76%   97.93%   +0.17%     
==========================================
  Files          36       36              
  Lines        2641     2910     +269     
  Branches      590      710     +120     
==========================================
+ Hits         2582     2850     +268     
+ Misses         52       51       -1     
- Partials        7        9       +2
Impacted Files Coverage Δ
src/lib/axes/axis_utils.ts 100% <100%> (ø) ⬆️
src/lib/series/specs.ts 100% <0%> (ø) ⬆️
src/lib/axes/canvas_text_bbox_calculator.ts 100% <0%> (ø) ⬆️
src/lib/utils/commons.ts 100% <0%> (ø) ⬆️
src/lib/series/rendering.ts 98.06% <0%> (+0.15%) ⬆️

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 fc6640d...5d33811. Read the comment docs.

src/lib/axes/axis_utils.ts Outdated Show resolved Hide resolved
@emmacunningham emmacunningham requested a review from markov00 July 2, 2019 06:21
Copy link
Collaborator

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

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

LGTM, tested and confirmed fix locally.

};
const xScale = getScaleForAxisSpec(horizontalAxisSpec, xBandDomain, [yDomain], 1, 0, 100, 0);
const histogramAxisPositions = getAvailableTicks(horizontalAxisSpec, xScale!, 1, enableHistogramMode);
const histogramTickLabels = histogramAxisPositions.map((tick: AxisTick) => tick.label);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think giving the type for HOF is a little redundant given the array should have the type, also destructuring. Just a thought, no big deal.

Suggested change
const histogramTickLabels = histogramAxisPositions.map((tick: AxisTick) => tick.label);
const histogramTickLabels = histogramAxisPositions.map(({ label }) => label);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the destructuring but kept the type annotation 5d33811

This is maybe something we will want to discuss more as a whole team, but in these kinds of cases, I prefer to see the type (even though it is not strictly necessary) because it helps me directly see the type of thing I'm working with without having to reference the array being mapped over; the explicitness isn't just for checking purposes but also reasoning purposes. Yes, it takes like milliseconds to check the type of the array and then infer the type of the function parameter, but making the type explicit frees up my working memory so that I don't have to do that inference myself. It's a small marginal benefit to be sure, but I'm not sure there's any disadvantage to having the type annotation here, so I'd take the small marginal benefit when I can.

Maybe this is something we can discuss in one of our upcoming syncs with @markov00?

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.

Changes LGTM!

@emmacunningham emmacunningham merged this pull request into elastic:master Jul 5, 2019
markov00 pushed a commit to markov00/elastic-charts that referenced this pull request Jul 5, 2019
markov00 pushed a commit that referenced this pull request Jul 5, 2019
# [7.2.0](v7.1.0...v7.2.0) (2019-07-05)

### Bug Fixes

* **ticks:** fill in additional ticks for histogram ([#251](#251)) ([af92736](af92736))

### Features

* **series:** stack series in percentage mode ([#250](#250)) ([1bfb430](1bfb430)), closes [#222](#222)
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
# [7.2.0](elastic/elastic-charts@v7.1.0...v7.2.0) (2019-07-05)

### Bug Fixes

* **ticks:** fill in additional ticks for histogram ([opensearch-project#251](elastic/elastic-charts#251)) ([8045531](elastic/elastic-charts@8045531))

### Features

* **series:** stack series in percentage mode ([opensearch-project#250](elastic/elastic-charts#250)) ([892a826](elastic/elastic-charts@892a826)), closes [opensearch-project#222](elastic/elastic-charts#222)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:axis Axis related issue bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants