fix(scales): use bisect to handle invertWithStep by markov00 · Pull Request #200 · elastic/elastic-charts

markov00 · 2019-04-30T10:00:23Z

Summary

close #195
close #183

We actually relay on the fact that data is evenly distributed among our x axis. We also rely on the minInterval value (the minimum interval between two consecutive x values on your dataset) to determine the current interval between x values.
Those wrong assumptions leads to issues when interacting with a chart: the interaction invert the pixel coordinate of the mouse to a scale value: the invertWithStep function to get the exact x value of the dataset for a bar chart with linear x axis, using the minInterval and the bandwidth to compute it. If we have odd placed x values or a domain that is larger than the current max x value, the tooltip and the highlighter are misaligned and wrongly displayed as in the closing issues #195 #183

Instead of relying on the minInterval, I've refactored the code to use a bisect function on invertWithStep, to get back the right x value from the chart.

I've also pixel adjusted few tests, that use the previous invertWithStep function.

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)
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
Each commit follows the convention

codecov-io · 2019-04-30T10:03:44Z

Codecov Report

❗ No coverage uploaded for pull request base (master@e11ec6a). Click here to learn what that means.
The diff coverage is 95.23%.

@@            Coverage Diff            @@
##             master     #200   +/-   ##
=========================================
  Coverage          ?   97.09%           
=========================================
  Files             ?       35           
  Lines             ?     1862           
  Branches          ?      256           
=========================================
  Hits              ?     1808           
  Misses            ?       47           
  Partials          ?        7

Impacted Files	Coverage Δ
src/lib/utils/scales/scale_band.ts	`92.3% <ø> (ø)`
src/state/chart_state.ts	`96.27% <100%> (ø)`
src/state/crosshair_utils.ts	`86% <100%> (ø)`
src/lib/utils/scales/scale_continuous.ts	`93.82% <93.75%> (ø)`

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 e11ec6a...d42bf42. Read the comment docs.

emmacunningham

tested locally & code lgtm, just a couple of minor comments about types

emmacunningham · 2019-04-30T15:43:23Z

src/lib/utils/scales/scale_continuous.test.ts

  });
  test('check if a scale is log scale', () => {
-    const domain = [0, 2];
+    const domain: any[] = [0, 2];


I think we have a Domain type that could be used here.

emmacunningham · 2019-05-01T16:51:19Z

src/state/crosshair_utils.ts

  const isHorizontalRotated = isHorizontalRotation(chartRotation);
  const snappedPosition = getSnapPosition(
-    xScale.invertWithStep(isHorizontalRotated ? x : y),
+    xScale.invertWithStep(isHorizontalRotated ? x : y, data),


Since invertWithStep may return undefined for continuous scales, do we need to update the type signature for getSnapPosition? Currently getSnapPosition defines value: string | number for its first parameter, but if the scale is a continuous scale, it's possible that the return value of invertWithStep is undefined. (I think we already handle if the scaled value is undefined, so the implementation doesn't need to change but rather just the type signature to more accurately reflect the possible values) Further, should we tighten the return type of invertWithStep in the Scale interface? Currently it is defined as (value: number, data: any[]) => any but getSnapPosition expects string | number.

As in its Scale parent class, invertWithScale return type is any because at runtime we can be never sure that the data object passed to the invertWithScale function contains only numbers. I've add few more checks on d42bf42 for getCursorBandPosition and for invertWithStep to avoid problems with undefined/null values. We should however check all the scales and the data types to have a correct handling of null/undefined values

👍 we could decode runtime values with something like io-ts but for now manually validating the data types within these functions works

codecov-io · 2019-05-02T13:15:23Z

Codecov Report

❗ No coverage uploaded for pull request base (master@e11ec6a). Click here to learn what that means.
The diff coverage is 88%.

@@            Coverage Diff            @@
##             master     #200   +/-   ##
=========================================
  Coverage          ?   96.99%           
=========================================
  Files             ?       35           
  Lines             ?     1865           
  Branches          ?      256           
=========================================
  Hits              ?     1809           
  Misses            ?       49           
  Partials          ?        7

Impacted Files	Coverage Δ
src/lib/utils/scales/scale_band.ts	`92.3% <ø> (ø)`
src/state/chart_state.ts	`96.27% <100%> (ø)`
src/state/crosshair_utils.ts	`84.31% <66.66%> (ø)`
src/lib/utils/scales/scale_continuous.ts	`92.77% <88.88%> (ø)`

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 e11ec6a...d42bf42. Read the comment docs.

## [4.0.1](v4.0.0...v4.0.1) (2019-05-02) ### Bug Fixes * **scales:** use bisect to handle invertWithStep ([#200](#200)) ([f971d05](f971d05)), closes [#195](#195) [#183](#183)

markov00 · 2019-05-02T16:29:00Z

🎉 This PR is included in version 4.0.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

## [4.0.1](elastic/elastic-charts@v4.0.0...v4.0.1) (2019-05-02) ### Bug Fixes * **scales:** use bisect to handle invertWithStep ([opensearch-project#200](elastic/elastic-charts#200)) ([d988e8b](elastic/elastic-charts@d988e8b)), closes [opensearch-project#195](elastic/elastic-charts#195) [opensearch-project#183](elastic/elastic-charts#183)

markov00 added 2 commits April 30, 2019 11:32

fix(scales): use bisect to handle invertWithStep

351192f

refactor(scale): add check for bisect on last value

e8bf412

markov00 added :interactions Interactions related issue bug Something isn't working labels Apr 30, 2019

markov00 requested a review from emmacunningham April 30, 2019 10:57

emmacunningham approved these changes May 1, 2019

View reviewed changes

markov00 added 2 commits May 2, 2019 14:56

refactor: add domain type to scale continuous test

fcd7afe

refactor: fix typings for invertWithScale

d42bf42

markov00 merged commit f971d05 into elastic:master May 2, 2019

markov00 deleted the bisect-linear-band-scale branch May 2, 2019 15:26

markov00 pushed a commit that referenced this pull request May 2, 2019

chore(release): 4.0.1 [skip ci]

2a4460e

## [4.0.1](v4.0.0...v4.0.1) (2019-05-02) ### Bug Fixes * **scales:** use bisect to handle invertWithStep ([#200](#200)) ([f971d05](f971d05)), closes [#195](#195) [#183](#183)

markov00 added the released Issue released publicly label May 2, 2019

This was referenced May 29, 2019

feat: add histogram mode #218

Merged

Handle discontinuous bar series crosshair interactions #221

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(scales): use bisect to handle invertWithStep#200

fix(scales): use bisect to handle invertWithStep#200
markov00 merged 4 commits intoelastic:masterfrom
markov00:bisect-linear-band-scale

markov00 commented Apr 30, 2019

Uh oh!

codecov-io commented Apr 30, 2019 •

edited

Loading

Uh oh!

emmacunningham left a comment

Uh oh!

emmacunningham Apr 30, 2019

Uh oh!

emmacunningham May 1, 2019

Uh oh!

markov00 May 2, 2019

Uh oh!

emmacunningham May 2, 2019

Uh oh!

codecov-io commented May 2, 2019 •

edited

Loading

Uh oh!

markov00 commented May 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

markov00 commented Apr 30, 2019

Summary

Checklist

Uh oh!

codecov-io commented Apr 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

emmacunningham left a comment

Choose a reason for hiding this comment

Uh oh!

emmacunningham Apr 30, 2019

Choose a reason for hiding this comment

Uh oh!

emmacunningham May 1, 2019

Choose a reason for hiding this comment

Uh oh!

markov00 May 2, 2019

Choose a reason for hiding this comment

Uh oh!

emmacunningham May 2, 2019

Choose a reason for hiding this comment

Uh oh!

codecov-io commented May 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

markov00 commented May 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-io commented Apr 30, 2019 •

edited

Loading

codecov-io commented May 2, 2019 •

edited

Loading