-
-
Notifications
You must be signed in to change notification settings - Fork 2k
Clickable Legend Titles #7698
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
base: master
Are you sure you want to change the base?
Clickable Legend Titles #7698
Conversation
…multiple legends
|
@alexshoe Can you document the full API for the new properties in this PR description? Basically just the info that's in the plot schema. |
|
@alexshoe Can you remove the image test? You can make a Codepen and link it in the PR description for testing purposes, but this feature doesn't require adding an image test to the test suite. |
| // Don't create a click targets for group titles when groupclick is 'toggleitem' | ||
| if(d[0].groupTitle && legendObj.groupclick === 'toggleitem') return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexshoe Is this line an unrelated bugfix for groupclick? Or is it related to the titleclick functionality?
(it's fine either way just want to clarify)
| if(numClicks === 1 && legendObj.titleclick) { | ||
| const clickVal = Events.triggerHandler(gd, 'plotly_legendtitleclick', evtData); | ||
| if(clickVal === false) return; | ||
|
|
||
| legendObj._titleClickTimeout = setTimeout(function() { | ||
| if(gd._fullLayout) handleTitleClick(gd, legendObj, legendObj.titleclick); | ||
| }, doubleClickDelay); | ||
| } else if(numClicks === 2) { | ||
| if(legendObj._titleClickTimeout) clearTimeout(legendObj._titleClickTimeout); | ||
| gd._legendMouseDownTime = 0; | ||
|
|
||
| const dblClickVal = Events.triggerHandler(gd, 'plotly_legendtitledoubleclick', evtData); | ||
| if(dblClickVal !== false && legendObj.titledoubleclick) handleTitleClick(gd, legendObj, legendObj.titledoubleclick); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexshoe Could some of the logic in the clickOrDoubleClick() function possibly be reused here? Seems like a fair amount of code duplication, although I'm sure there are some subtle differences.
| function positionTitleToggle(scrollBox, legendObj, legendId) { | ||
| const titleToggle = scrollBox.select('.' + legendId + 'titletoggle'); | ||
| if(!titleToggle.size()) return; | ||
|
|
||
| const side = legendObj.title.side || 'top'; | ||
| const bw = legendObj.borderwidth; | ||
| var x = bw; | ||
| const width = legendObj._titleWidth + 2 * constants.titlePad; | ||
| const height = legendObj._titleHeight + 2 * constants.titlePad; | ||
|
|
||
|
|
||
| if(side === 'top center') { | ||
| x = bw + 0.5 * (legendObj._width - 2 * bw - width); | ||
| } else if(side === 'top right') { | ||
| x = legendObj._width - bw - width; | ||
| } | ||
|
|
||
| titleToggle.attr({ x: x, y: bw, width: width, height: height }); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, likewise this function seems like it's duplicating a lot of the title placement logic. I don't think we should be referencing parameters like legendObj.title.side here at all. My feeling is that the titleToggle logic should be more parallel to the traceToggle placement logic.
| const legendItem = g.data()[0][0]; | ||
| if(legendItem.groupTitle && legendItem.noClick) return; | ||
|
|
||
| const legendId = legendItem.trace.legend || 'legend'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, this fixes a bug where 'secondary' legends (e.g. legend2, legend3, etc.) used the click settings of legend rather than using their own settings, is that right? I've tried it out in Codepen and that seems to be the case.
I believe that fixes this issue, can you confirm @alexshoe ? If so, please tag in PR description.
| exports.handleTitleClick = function handleTitleClick(gd, legendObj, mode) { | ||
| const fullLayout = gd._fullLayout; | ||
| const fullData = gd._fullData; | ||
| const legendId = legendObj._id || 'legend'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps move the getId() function in src/components/legend/draw.js to helpers.js and use it here, since it does exactly this
| } | ||
| }; | ||
|
|
||
| exports.handleTitleClick = function handleTitleClick(gd, legendObj, mode) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also similar comment here to above -- check whether you can share some of this logic with the handleClick function to avoid TOO much code duplication
| } | ||
| }; | ||
|
|
||
| exports.handleTitleClick = function handleTitleClick(gd, legendObj, mode) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does bother me that this function signature isn't the same as the one for handleClick() -- maybe this signature is better and/or more appropriate for legend titles! But just double check that the signature makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like for the sake of consistency, would it be better to accept numClicks here? Or alternatively modify handleClick() to accept a mode argument?
| var toggleThisLegend; | ||
| var toggleOtherLegends; | ||
|
|
||
| if(mode === 'toggle') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's unclear what are the possible values for mode. Can you add a docstring for the handleTitleClick function?
| }); | ||
|
|
||
| toggleThisLegend = !anyVisibleHere; | ||
| toggleOtherLegends = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| toggleOtherLegends = null; | |
| toggleOtherLegends = false; |
I don't see why you wouldn't make this false
|
|
||
| for(var i = 0; i < allLegendItems.length; i++) { | ||
| const item = allLegendItems[i]; | ||
| const shouldShow = isInLegend(item) ? toggleThisLegend : toggleOtherLegends; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this right? I feel like this might give the wrong result when item.showlegend is false. I can try to come up with a reproducible example if that would help.
Description:
Adds the ability to click/double-click on legend titles to toggle legend visibility, making it easier to manage complex charts with multiple legends.
Example:
See this codepen for an interactive demo of this feature
New API:
legend.titleclick'toggle'|'toggleothers'|false'toggle'when there are multiple legends,falseotherwiselegend.titledoubleclick'toggle'|'toggleothers'|false'toggleothers'when there are multiple legends,falseotherwiseWhat each value for
titleclickandtitledoubleclickdoes:'toggle'toggles the visibility of all items in the clicked legend'toggleothers'toggles the visibility of all other legendsfalsedisables legend title double-click interactions