Skip to content

Add option to include invisible points#10362

Merged
kurkle merged 8 commits into
chartjs:masterfrom
yhvicey:AddOptionToIncludeInvisible
May 25, 2022
Merged

Add option to include invisible points#10362
kurkle merged 8 commits into
chartjs:masterfrom
yhvicey:AddOptionToIncludeInvisible

Conversation

@yhvicey

@yhvicey yhvicey commented May 19, 2022

Copy link
Copy Markdown
Contributor

Fixes #10361.

@LeeLenaleee

Copy link
Copy Markdown
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

@kurkle kurkle left a comment

Copy link
Copy Markdown
Member

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

yhvicey commented May 24, 2022

Copy link
Copy Markdown
Contributor Author

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

kurkle commented May 24, 2022

Copy link
Copy Markdown
Member

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"?

Comment thread docs/configuration/interactions.md Outdated
Comment thread src/core/core.interaction.js Outdated
Comment thread src/core/core.interaction.js Outdated
Comment thread types/index.esm.d.ts Outdated
Comment thread test/specs/core.interaction.tests.js Outdated
Comment thread src/core/core.interaction.js Outdated
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>

@etimberg etimberg left a comment

Copy link
Copy Markdown
Member

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
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