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 compute bar width #1663

Merged
merged 7 commits into from
Jun 18, 2019
Merged

Fix compute bar width #1663

merged 7 commits into from
Jun 18, 2019

Conversation

andrewdove1
Copy link
Member

This fixes an issue where if there are duplicate categories in a category axis the bar width ends up being zero.

@andrewdove1 andrewdove1 requested a review from atmgrifter00 June 12, 2019 16:55
xValues = xValues.filter( onlyUnique );

for (var j = 1; j < xValues.length; j++) {
distance = Math.abs(xValues[j] - xValues[j - 1]);
Copy link
Contributor

@atmgrifter00 atmgrifter00 Jun 12, 2019

Choose a reason for hiding this comment

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

So, in our discussion I was under the impression that xValues was sorted AND filtered for unique values, but this only seems to be filtered for unique values. Sorting still seems important for the following scenario:

data input:
{ {3, 5, 0}, {8, 2, 0}, {1, 2, 0} } (So, x-values of 3, 8, 1).

The algorithm as written will come up with a distance of 5. Whereas with sorting it will come up with a distance of 2.

Edit: I just saw that you removed a change that was, indeed, doing the sort. Why did you decide it wasn't needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

In the end I decided that eliminating duplicates was the way to go. That is what the category axis itself does. Sorting yielded incorrect results.

Copy link
Contributor

Choose a reason for hiding this comment

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

After reviewing the behavior described above, Andrew and I are in agreement that the preferred behavior requires the x-values to be sorted. Had to update a test as a result of that.

Jonathan Meyer added 3 commits June 18, 2019 10:52
Sort the x-values before computing bar width.
* master:
  Reverting changes for loading CORS stylesheets (PRs 1651 and 1652) (#1666)
  Explicity perform null check on bar hover index (#1644)
  Reset to 1 (#1665)
  Fixed microsecond issue. (#1660)
  Add categories to build (#1659)
  Fix issue #1642 (#1657)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants