-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Conversation
src/components/legend/style.js
Outdated
@@ -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); |
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.
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?
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.
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.
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.
Addressed in 10d1138.
FYI not new, I'm just noticing it here, there's an inconsistency in 4 mocks "mode": "lines+markers+text",
"name": "Lines and Text", |
Good eye. Fixed in 0bd9660. |
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.
💃 looks good. Thanks for updating those inconsistent mocks 🎉
Resolves #5679.
@plotly/plotly_js