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

do not delete hoverlabel when hovertemplate is defined #3480

Merged
merged 3 commits into from
Jan 28, 2019

Conversation

antoinerg
Copy link
Contributor

Closes #3478

@etpinard etpinard added bug something broken status: reviewable labels Jan 24, 2019
@etpinard
Copy link
Contributor

Nice!

It might be nice to add a few more test cases 🔒 ing down this fix. Maybe one for scattergeo, one for scatterpolar, one for scatterternary and one scattermapbox? That is, one test for each subtype type supported by hovertemplate.

@antoinerg
Copy link
Contributor Author

Just for future reference, problem #3418 fixed by the current PR will arise for any trace types that do not append anything to variable text after going through:

if(d.zLabel !== undefined) {
if(d.xLabel !== undefined) text += 'x: ' + d.xLabel + '<br>';
if(d.yLabel !== undefined) text += 'y: ' + d.yLabel + '<br>';
text += (text ? 'z: ' : '') + d.zLabel;
}
else if(showCommonLabel && d[hovermode + 'Label'] === t0) {
text = d[(hovermode === 'x' ? 'y' : 'x') + 'Label'] || '';
}
else if(d.xLabel === undefined) {
if(d.yLabel !== undefined) text = d.yLabel;
}
else if(d.yLabel === undefined) text = d.xLabel;
else text = '(' + d.xLabel + ', ' + d.yLabel + ')';
if((d.text || d.text === 0) && !Array.isArray(d.text)) {
text += (text ? '<br>' : '') + d.text;
}
// used by other modules (initially just ternary) that
// manage their own hoverinfo independent of cleanPoint
// the rest of this will still apply, so such modules
// can still put things in (x|y|z)Label, text, and name
// and hoverinfo will still determine their visibility
if(d.extraText !== undefined) text += (text ? '<br>' : '') + d.extraText;
// if 'text' is empty at this point,
// put 'name' in main label and don't show secondary label
if(text === '') {
// if 'name' is also empty, remove entire label
if(name === '') g.remove();
text = name;
}

@etpinard
Copy link
Contributor

@antoinerg can you try to add tests for scatterternary and scattermapbox at some point today? I'd like to include this PR in today's (or tomorrow's 😏 ) v1.44.2. Thanks!

@antoinerg
Copy link
Contributor Author

antoinerg commented Jan 28, 2019

@antoinerg can you try to add tests for scatterternary and scattermapbox at some point today? I'd like to include this PR in today's (or tomorrow's ) v1.44.2. Thanks!

This is now done in 7bff254. Sorry for the slow reply: I initially struggled to reproduce the issue for scatterternary and scattermapbox before realizing the existing tests didn't actually check the content or the existence of hover labels on the page.

@etpinard
Copy link
Contributor

Thanks 💃 !

@antoinerg antoinerg merged commit ea7941b into master Jan 28, 2019
@antoinerg antoinerg deleted the 3478-fix-hovertemplate branch January 28, 2019 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants