Skip to content
This repository has been archived by the owner on Jul 29, 2019. It is now read-only.

Toggle Group Visibility #2303

Closed
wants to merge 1 commit into from
Closed

Conversation

simo9000
Copy link
Contributor

This is an implementation of the feature requested in #257. This only toggles group visibility not subgroup visibility. This also includes and update to the documentation and an example page to highlight the new feature.

* Added group option to change the visibility of a group
* updated the groupsEditable example page to demo the use of this feature
* updated the timeline documentation to describe this option
Copy link
Contributor

@yotamberk yotamberk left a comment

Choose a reason for hiding this comment

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

Nice! Thanks for the PR!
The thing is, I don't see how this feature is needed. A user can simply add to their group an attribute of visible:false dynamically and simply set the groups with the filtered groups (e.g. using groups.filter or with the dataset filter). Not huge benefit to this feature but I'm willing to approve it. Thanks for the contribution!



// loop through groups and convert content strings into elements with hide buttons.
groups.forEach(function(group){
Copy link
Contributor

Choose a reason for hiding this comment

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

You're better off using the options.groupTemplate to achieve this. This seems kind of hacky and unclear...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hadn't considered that. I'll give that a shot.

@simo9000
Copy link
Contributor Author

I tried your idea with the dataset.filter and it works just as well as the PR. It is probably a better solution since it likely has less overhead for hidden groups. What i'll do then is break up the PR to remove the changes to the library and instead i'll modify the example to use the dataset.filter. Then #257 can reference the updated example and be closed.

@yotamberk
Copy link
Contributor

So I'll close this PR and when you're ready with a new one, please resubmit =)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants