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

Add option to include invisible points #10362

Merged
merged 8 commits into from May 25, 2022

Conversation

yhvicey
Copy link
Contributor

@yhvicey yhvicey commented May 19, 2022

Fixes #10361.

@LeeLenaleee
Copy link
Collaborator

Haven't looked at code, but the new option should also be documented in the documentation itself instead of only in the typings

@etimberg etimberg requested a review from kurkle May 22, 2022 15:07
Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

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

Only doubt is the option name. It only includes invisible points outside chart area (and those could still be visible if clipping is disabled). But it does not include hidden points inside chart area, right?

@yhvicey
Copy link
Contributor Author

yhvicey commented May 24, 2022

I just added an option to suppress isPointInArea check - So I guess it won't impact hidden points? I'm not sure if this can be tested via UT, is there any existing example to put points into hidden state in UT?

@kurkle
Copy link
Member

kurkle commented May 24, 2022

I just added an option to suppress isPointInArea check - So I guess it won't impact hidden points? I'm not sure if this can be tested via UT, is there any existing example to put points into hidden state in UT?

Entire dataset can be hidden by adding hidden: true to that dataset. Though I'm sure the change does not impact those points.

I fail to come up with a better name for the option, so maybe just update the description to mention "outside chartArea"?

docs/configuration/interactions.md Outdated Show resolved Hide resolved
src/core/core.interaction.js Outdated Show resolved Hide resolved
src/core/core.interaction.js Outdated Show resolved Hide resolved
types/index.esm.d.ts Outdated Show resolved Hide resolved
test/specs/core.interaction.tests.js Outdated Show resolved Hide resolved
src/core/core.interaction.js Outdated Show resolved Hide resolved
yhvicey and others added 2 commits May 24, 2022 22:03
Co-authored-by: Jacco van den Berg <39033624+LeeLenaleee@users.noreply.github.com>
Co-authored-by: Jacco van den Berg <39033624+LeeLenaleee@users.noreply.github.com>
Copy link
Member

@etimberg etimberg left a comment

Choose a reason for hiding this comment

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

I'm ok with includeInvisible as the param name. I didn't notice anything with the code

@kurkle kurkle requested a review from LeeLenaleee May 25, 2022 10:18
@kurkle kurkle merged commit ebcaff1 into chartjs:master May 25, 2022
@yhvicey yhvicey deleted the AddOptionToIncludeInvisible branch May 25, 2022 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add option for interactions to allow tooltips include invisible points
5 participants