Add legend.itemheight to set the height of the legend fill swatch - #7925
Open
MannXo wants to merge 2 commits into
Open
Add legend.itemheight to set the height of the legend fill swatch#7925MannXo wants to merge 2 commits into
MannXo wants to merge 2 commits into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #7924
Adds
layout.legend.itemheight, the vertical counterpart to the existingitemwidth, so a taller legend swatch can reveal more of a trace'sfillpattern.The two parts
The fill swatch height was hardcoded in
src/components/legend/style.js:That
v6is the 6 pixels mentioned in the issue. Making it configurable on its own is not enough, though: legend row height is computed indraw.jsasMath.max(height, 16) + 3, so anything taller than about 13px would have overlapped its neighbours. That is most likely the "my vertical alignment is off" in the original report. This PR changes both:attributes.js/defaults.js: newitemheight,min: 6,dflt: 6,editType: 'legend'.min === dfltmirrorsitemwidthexactly (min: 30, dflt: 30).style.js:v6becomesv<itemheight>.draw.js: row height becomesMath.max(height, 16, legendObj.itemheight + 10) + 3, so items grow to fit the swatch.At the default the rendering is unchanged. Since
6 + 10 === 16, the new term cannot win atitemheight: 6, and the swatch path is stillv6. I kept the literal16as an independent floor rather than replacing it withitemheight + 10, because those two being equal at the default is a choice of constant on my part, not something I can show about why16was originally picked.The swatch grows downwards, leaving
pathStartalone. That keeps the trace line sitting on the top edge of the fill, which is how a filled area actually renders in the plot, and it matches the mock in the issue. It also means a trace with no fill is completely unaffected, sincepathStartis what positions the line.Unified hover labels need no separate handling:
hover.jsbuilds itsmockLegendthroughlegendSupplyDefaults, so the new attribute is coerced there too.Before / after
Three
fillpatterntraces, default versuslegend.itemheight: 24. The/,xand.patterns are hard to tell apart at 6px and legible at 24px.Testing
npm run test-jasmine -- legend --nowatchgives 132 passing, of which 8 are new.hover_labelis 221 passing.npm run lint,npm run test-syntaxandnpm run test-mockare all clean.test/plot-schema.jsonregenerated withnpm run schema;dist/deliberately untouched.New tests:
reproduces the historical 6px swatch at the defaultM5,0h30v6h-30zso the default cannot driftgrows the fill swatch to the requested heightgrows the swatch downwards, leaving the line on top of the filldoes not move the swatch of a trace without fillgrows each legend row so taller swatches do not overlapdraw.jshalfleaves row heights untouched at the defaultapplies to unified hover labels without errormockLegendpathdefaults itemheight to 6 and clamps values below the minimumI verified the tests actually bind to the change rather than passing regardless. Reverting only the
style.jsline fails exactly the two swatch-growth tests and nothing else; reverting only thedraw.jsline fails exactly the row-growth test. The default-behaviour guards pass either way by design, which is what stops the feature from degenerating into "move the swatch".Three things worth a maintainer's call
itemheightcurrently drives only the fill swatch. Bar and marker swatches have their own fixed geometry (styleBarsdrawsM6,6H-6V-6H6Z, a 12x12 square). Shoulditemheightdrive those too, or stay fill-only as the issue requests?min: 6. Chosen to match today's height and to mirroritemwidth'smin === dflt. Happy to lower it if you would rather allow thinner swatches.legend.valignalready offers some control here and the default is untouched, so I left text positioning alone rather than adding a second alignment mechanism. Say the word if you would prefer the label centred on the swatch.No image mock is included yet. The
itemwidthprecedent istest/image/mocks/legend_itemwidth_dashline.json, so I am happy to add one, but since baselines are generated onubuntu-latestI would rather take the baseline from CI than commit a macOS render. Let me know and I will add it.