The Wayback Machine - https://web.archive.org/web/20211017161224/https://github.com/plotly/plotly.js/pull/5849
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

Jump to bottom

DRAFT: add legend.groupclick option #5849

Closed
wants to merge 1 commit into from
Closed

DRAFT: add legend.groupclick option #5849

wants to merge 1 commit into from

Conversation

@brussee
Copy link
Contributor

@brussee brussee commented Jul 21, 2021

toggle individual items in a legendgroup or the legendgroup as a whole

Fixes #3135

This is a draft. I can add tests if there is consensus about the approach taken here. Also this is my first PR to this repo, please forgive me if I made rookie mistakes :)

TODO:

  • Tests

After opening a pull request, developer:

  • should create a new small markdown log file using the PR number e.g. 1010_fix.md or 1010_add.md inside draftlogs folder as described in this README, commit it and push.
  • should not force push (i.e. git push -f) to remote branches associated with opened pull requests. Force pushes make it hard for maintainers to keep track of updates. Therefore, if required, please fetch upstream/master and "merge" with master instead of "rebase".
@brussee
Copy link
Contributor Author

@brussee brussee commented Jul 21, 2021

@nicolaskruchten

@nicolaskruchten
Copy link
Member

@nicolaskruchten nicolaskruchten commented Jul 21, 2021

Thanks for this PR! This is definitely the right approach! The next step would be getting the CircleCI build to go green (see github status block above) and then we can look at which tests make sense.

@brussee brussee force-pushed the feature/legendgrouptoggle branch 3 times, most recently from 82b453a to 3f739a3 Jul 28, 2021
@brussee
add legend.groupclick option
b0ac05f 
toggle individual items in a legendgroup or the legendgroup as a whole
@brussee brussee force-pushed the feature/legendgrouptoggle branch from 3f739a3 to b0ac05f Jul 28, 2021
@brussee
Copy link
Contributor Author

@brussee brussee commented Jul 29, 2021

Rebased on master.

@nicolaskruchten
Copy link
Member

@nicolaskruchten nicolaskruchten commented Aug 24, 2021

This looks like an awesome PR, thank you! I'm sorry for the long delay in responding :)

@alexcjohnson can we get a review on this one please?

@archmoj
Copy link
Collaborator

@archmoj archmoj commented Aug 24, 2021

Please create draftlogs/5849_add.md as described in this README.
Thank you!

@archmoj archmoj added this to the v2.4.0 milestone Aug 24, 2021
alexcjohnson
alexcjohnson reviewed Aug 24, 2021
View changes
src/components/legend/handle_click.js
@@ -27,6 +28,8 @@ module.exports = function handleClick(g, gd, numClicks) {
else if(numClicks === 2) mode = itemDoubleClick;
if(!mode) return;

var toggleGroup = groupClick === undefined || groupClick === 'togglegroup';
Copy link
Contributor

@alexcjohnson alexcjohnson Aug 24, 2021

groupclick has a dflt value, so is there ever a case where we still get undefined?

Copy link
Contributor Author

@brussee brussee Aug 26, 2021

No defined case indeed. (So only if it is possible to change this value manually or in case of a bug.)

alexcjohnson
alexcjohnson reviewed Aug 24, 2021
View changes
src/components/legend/attributes.js
'Determines the behavior on legend group item click.',
'*toggleitem* toggles the visibility of the individual item clicked on the graph.',
'*togglegroup* toggles the visibility of all items in the same legendgroup as the item clicked on the graph.',
'*false* disables legend group click interactions.'
Copy link
Contributor

@alexcjohnson alexcjohnson Aug 24, 2021

What's the use case for groupclick: false that isn't covered by itemclick: false? I guess you have some traces in a group and others ungrouped, and you want to allow clicks on the ungrouped traces but not on the groups? That feels weird. Unless I'm missing something, can we remove false as an option here?

Copy link
Member

@nicolaskruchten nicolaskruchten Aug 24, 2021

I tend to agree that false is not a valid option here, and that groupclick should only determine what happens when you click on a grouped item AND itemclick is not false

Copy link
Contributor Author

@brussee brussee Aug 26, 2021

A better way to 'lock' certain traces/legendgroups would be with a dedicated feature, so I agree to remove the "false" option here.

@alexcjohnson
Copy link
Contributor

@alexcjohnson alexcjohnson commented Aug 24, 2021

Looks great, thanks for the typo fixes! Aside from the comments above, it would be nice to include a test of the new behavior - I guess just a variant of the legendgroup visibility test with groupclick: 'toggleitem'

@archmoj
Copy link
Collaborator

@archmoj archmoj commented Aug 25, 2021

@brussee wondering if you would possibly be able to get this PR to the finish line today? If not I'd be happy to help with that.

@archmoj archmoj mentioned this pull request Aug 25, 2021
Merged
@archmoj archmoj removed this from the v2.4.0 milestone Aug 25, 2021
@archmoj
Copy link
Collaborator

@archmoj archmoj commented Aug 25, 2021

Thanks @brussee.
Closing in favor of follow up pull: #5906

@archmoj archmoj closed this Aug 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolaskruchten nicolaskruchten

@alexcjohnson alexcjohnson

Assignees
No one assigned
Labels
type: community type: feature
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Single-toggle of grouped traces in legend
4 participants
@brussee @nicolaskruchten @archmoj @alexcjohnson