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 show Aa text in legends unless there is only text in the legend item #5682

Merged
merged 7 commits into from
May 27, 2021
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/components/legend/style.js
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ module.exports = function style(s, gd, legend) {

// with fill and no markers or text, move the line and fill up a bit
// so it's more centered
var markersOrText = subTypes.hasMarkers(trace) || subTypes.hasText(trace);
var markersOrText = subTypes.hasMarkers(trace) || subTypes.isText(trace);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe this change is correct, though I don't see any test images in which the legend symbol shifted position due to the text being removed. Probably OK, just want to note it as a gap. This would need to be something like mode: 'lines+text', fill: 'tozeroy'.

Actually this raises a question: what if we have just fill and text, mode: 'text', fill: 'tozeroy' - do we still want to show the Aa in that case, or only the fill? Currently in this PR we would still show Aa but I might argue for dropping it, ie showing text only when there's nothing else at all in the legend. @nicolaskruchten thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, let's make this rule "if rendering no text would result in no legend item at all, show the text". So if fills without lines cause a marker to appear, then no text please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 10d1138.

var anyFill = showFill || showGradientFill;
var anyLine = showLine || showGradientLine;
var pathStart = (markersOrText || !anyFill) ? 'M5,0' :
Expand Down Expand Up @@ -187,7 +187,7 @@ module.exports = function style(s, gd, legend) {
var d0 = d[0];
var trace = d0.trace;
var showMarkers = subTypes.hasMarkers(trace);
var showText = subTypes.hasText(trace);
var showText = subTypes.isText(trace);
var showLines = subTypes.hasLines(trace);
var dMod, tMod;

Expand Down
4 changes: 4 additions & 0 deletions src/traces/scatter/subtypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ module.exports = {
trace.mode.indexOf('text') !== -1;
},

isText: function(trace) {
return trace.visible && trace.mode === 'text';
},

isBubble: function(trace) {
return Lib.isPlainObject(trace.marker) &&
Lib.isArrayOrTypedArray(trace.marker.size);
Expand Down
Binary file modified test/image/baselines/cliponaxis_false-dates-log.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/image/baselines/geo_africa-insets.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/image/baselines/geo_legendonly.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/image/baselines/gl2d_point-selection.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/image/baselines/gl2d_text_chart_arrays.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/image/baselines/gl2d_text_chart_invalid-arrays.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/image/baselines/gl3d_scatter3d-align-texts.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/image/baselines/gl3d_scatter3d-blank-text.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/image/baselines/gl3d_scatter3d-texttemplate.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/image/baselines/legend-constant-itemsizing.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/image/baselines/mathjax.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/image/baselines/point-selection.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/image/baselines/scattercarpet-text.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/image/baselines/text_chart_arrays.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/image/baselines/text_chart_invalid-arrays.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/image/baselines/texttemplate.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
3 changes: 0 additions & 3 deletions test/jasmine/tests/scatter_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -796,7 +796,6 @@ describe('end-to-end scatter tests', function() {
var hasFills = name.indexOf('fill') !== -1;
var hasLines = name.indexOf('lines') !== -1;
var hasMarkers = name.indexOf('markers') !== -1;
var hasText = name.indexOf('text') !== -1;
var tracei, prefix;

// construct the expected ordering based on case name
Expand All @@ -814,7 +813,6 @@ describe('end-to-end scatter tests', function() {
}
if(hasLines) selectorArray.push(prefix + '.js-line');
if(hasMarkers) selectorArray.push(prefix + '.point');
if(hasText) selectorArray.push(prefix + '.textpoint');
}

// ordering in the legend
Expand All @@ -823,7 +821,6 @@ describe('end-to-end scatter tests', function() {
if(hasFills) selectorArray.push(prefix + '.js-fill');
if(hasLines) selectorArray.push(prefix + '.js-line');
if(hasMarkers) selectorArray.push(prefix + '.scatterpts');
if(hasText) selectorArray.push(prefix + '.pointtext');
}

var msg = i ? ('from ' + cases[indices[i - 1]].name + ' to ') : 'from default to ';
Expand Down