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 API to change data visibility #6907

Merged
merged 1 commit into from Jan 6, 2020

Conversation

etimberg
Copy link
Member

@etimberg etimberg commented Jan 4, 2020

This came up when removing the HTML legend. Adds two new public methods to the core Chart class for setting data & dataset visibility.

@kurkle
Copy link
Member

kurkle commented Jan 5, 2020

Oh, I so hate long function names. It could be even clearer if called setDatasetVisibilityAtIndex 😈 (j/k)

Seriously though, one of the argument behind setVisibility vs show / hide is less functions. If that's what we are looking for, why not just setVisibility(datasetIndex, index), where you can omit the index if setting for dataset?

Another argument is the usage pattern:

setVisible(shouldItBeVisible())

vs

if (itShouldBeVisible()) {
  show();
} else {
  hide();
}

Add animations to those:

let visible = shouldItBeVisible();
setVisibility(datasetIndex, index, visible);

// need to add some parameters to limit the update to those correct elements. 
// `update` should not be a side effect of `set` function.
update(visible?'show':'hide', datasetIndex, index);

vs

if (itShouldBeVisible()) {
  show(datasetIndex, index); // update can be a side effect
} else {
  hide(datasetIndex, index);
}

And finally, I can live with the proposed ones, just wanted to share my thoughts.

Copy link
Contributor

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

I like the proposed API in this PR

I still don't love the implementation of adding a hidden field and making everything check for it. I feel like it should just remove an element from _parsed and store it elsewhere to be restored rather than littering the rest of the library with if (!data.hidden) checks

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.

None yet

3 participants