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
Interaction functions #10046
Interaction functions #10046
Conversation
To work toward exposing something like the get...Items functions.
optimizedEvaluateItems falls back to evaluating all items for unsorted items, and sorting / optimizing ought to be okay, so this ought to be equivalent.
helpers.dom.js's getRelativePosition already had logic to handle ChartEvent vs. Event (as demonstrated by the `native` check within `getCanvasPosition`), so it's redundant for core.interaction.js to have its own `native` check. Update `getRelativePosition` to use the same `native` check as core.interaction.js's version. As best as I can tell, the ChartEvent's x and y are populated from `getRelativePosition`, so the previous `getCanvasPosition` was effectively just duplicating `getRelativePosition'`s work. I added a test to verify this; it depends on a local, not-yet-submitted change in chartjs-test-utils' `triggerMouseEvent` to return the mouse event that it triggers.
Would it actually be enough to only export these "higher level" functions? Would probably leave more room for internal changes. |
@kurkle For my purposes, I don't think that would help. Going off of the two motivating examples from #9961 (
(I realize that there's discussion of other changes that might remove the need for either or both of these specific examples, but hopefully the examples can still demonstrate what might make for a more broadly useful API.) |
I don't have any objections to this. |
There is a change pending release on the test-utils, could also include your changes if you open the PR. |
@kurkle Submitted as chartjs/chartjs-test-utils#19. Thanks for the reminder. For this PR: I listed some optional functions for export as items 4 and 5 (the individual Thank you. |
This allows calling code to do additional tests on the event; see chartjs/Chart.js#10046
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few questions / comments. Overall I like the direction this is going
Add a non-null assertion, as requested in code review. Add JSDoc to clarify that `getCanvasPosition` now expects a native `Event`, not a `ChartEvent`. Add `@ts-ignore`; `getCanvasPosition` relied on some loose conversions between `Event`, `TouchEvent`, and `Touch` that would require several changes to make TypeScript happy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of nits
|
Return the event directly, to speed up the code a bit. Add JSDoc to help communicate its intent. Update various comments.
@kurkle Are you seeing this error? If so, where? |
@kurkle and @etimberg , please see my latest commits:
|
CI fails with that. I guess the event can be undefined in some case. |
@kurkle looking at CI history ( https://github.com/chartjs/Chart.js/actions?query=workflow%3ACI+branch%3Amaster ) the tests seem to not be 100% deterministic, they sometimes fail randomly on master. The changes submitted by @joshkel (thank you for PR!) don't seem to touch code that failed tests in CI. Is it possible to rerun tests? I would love to see this PR merged and released as we also need the "nearestPerDataset" mode in our project. |
Only thing needed actually is the update of chartjs-test-utils to 0.4.0
So the CI fails because of old chartjs-test-utils that does not return an event. @joshkel can you update that? (not easy enough for me to do) |
To get supporting work from chartjs/chartjs-test-utils#19
@kurkle Done. Thanks for following up on this, and sorry for the delay. |
@kurkle am I OK to merge this? |
Sure, I only closed/re-opened for actions to restart (they were down yesterday) |
For discussion / work in progress - cleaning up and expanding the interaction modes API, as discussed in #9961.
Specific changes:
getRelativePosition
. See c765938 for details. I added a test to verify this; it depends on a local, not-yet-submitted change in chartjs-test-utils'triggerMouseEvent
to return the mouse event that it triggers. I'll open a separate PR for that if this change seems reasonable.Chart
method,isPointInArea
, to refactor and publicize logic that was duplicated in core.controller.js and core.interactions.js.getAxisItems
to have an interface consistent with otherget...Items
functions.evaluateAllVisibleItems
andoptimizedEvaluateItems
-optimizeEvaluateItems
falls back to evaluating everything if the optimization fails, and I couldn't think of a reason whygetAxisItems
wouldn't want to benefit from the optimized binary search if it could.Proposed API:
getRelativePosition
is already exported by'chart.js/helpers'
and, after these changes, can efficiently be used as is.Chart
method,isPointInArea
.optimizedEvaluateItems
toevaluateInteractionItems
and export it.get...Items
functions:getIntersectItems
,getNearestItems
,getAxisItems
. (I went ahead and marked these andevaluateInteractionItems
asexport
in the current PR.)getDistanceMetricForAxis
.I'm currently thinking that the
get...Items
andgetDistanceMetricForAxis
functions would not be exported: They're straightforward to implement using existing public APIs, and they may need to vary due to specific interaction needs, so the costs of freezing them as part of the public API may not outweigh the benefits.For reference, if I were to export some sort of
get...Items
functionality, here's what I'm using for my own code. It's based off ofgetNearestItems
, modified to take a callback: (This function could be exported, andgetNearestCartesianItems
could be changed to use this.)