Conversation
|
Some thoughts: instead of duplicating modes for both axes, maybe we should introduce a new interaction options to select the orientation (
Could also apply to nearest and index:
(though not sure |
|
@simonbrunel that would work. Axis x would do the current behaviour, axis 'y' would do the new behaviour. I don't know that 'xy' is needed but we could add it for completeness |
|
Looks good (unit tests are failing though). Do we want unit tests for this new options? |
|
I thought I fixed those tests. will check again |
etimberg
force-pushed
the
fix/3905
branch
2 times, most recently
from
13abff7 to
de1cc57
Compare
|
@simonbrunel i added some tests for |
test/specs/core.interaction.tests.js
Outdated
| } | ||
| describe ('intersect: false', function() { | ||
| it ('axis: x gets correct items', function() { | ||
| var chart = window.acquireChart({ |
There was a problem hiding this comment.
Since these tests use the same chart input, we could refactor it in beforeEach() under the describe ('intersect: false'...` block?
| var node = chart.canvas; | ||
| var rect = node.getBoundingClientRect(); | ||
|
|
||
| var evt = { |
There was a problem hiding this comment.
Might be easier and more accurate to generate a Chart.js event instead (nativeEvent: null)?
|
Tests look fine, I would maybe also add tests for cases where it's supposed to return no elements (I don't know it that makes sense). |
|
Updated per the comments. I cleaned up all the tests to use |
…ts instead of native events. Added more tests for axis modes
test/specs/core.interaction.tests.js
Outdated
| var chart; | ||
|
|
||
| beforeEach(function() { | ||
| chart = window.acquireChart({ |
There was a problem hiding this comment.
I think it's a bit cleaner to use this.chart instead of a global variable.
simonbrunel
left a comment
simonbrunel
left a comment
There was a problem hiding this comment.
Looks good (just a minor comment), I rebased the PR since it was failing because of Chrome 60.
|
Awesome! Updated per your comment 😄 |
chartjs#4458) index-y interaction mode + convert horizontal bar defaults to new mode
chartjs#4458) index-y interaction mode + convert horizontal bar defaults to new mode
2 participants
This new mode resolves #3905 and works much better for a horizontal bar chart when
intersect: false