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.

Sign up for GitHub

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

Jump to bottom

Add title feature to legend and support legend option for various traces #4386

Merged
merged 7 commits into from
Dec 2, 2019
Merged

Add title feature to legend and support legend option for various traces #4386

merged 7 commits into from
Dec 2, 2019

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Nov 25, 2019 •
edited by nicolaskruchten
Loading

resolves #276 and
resolves #2642 and
resolves #3468

List of new layout attributes:

  • legend.title
  • legend.title.text
  • legend.title.font
  • legend.title.side

List of new traces with legend:

  • choropleth
  • choroplethmapbox
  • cone
  • densitymapbox
  • heatmap
  • histogram2d
  • isosurface
  • mesh3d
  • streamtube
  • surface
  • volume

@plotly/plotly_js
cc: @nicolaskruchten

before vs after demo

archmoj and others added 3 commits November 25, 2019 14:44
@archmoj
add title to legend
9174470
@archmoj
add option to enable legends for
706a883 
- choropleth
- choroplethmapbox
- cone
- densitymapbox
- heatmap
- histogram2d
- isosurface
- mesh3d
- streamtube
- surface
- volume
@archmoj
add new legend mocks
264044f
@archmoj archmoj added this to the v1.52.0 milestone Nov 25, 2019
etpinard
etpinard reviewed Nov 26, 2019
View reviewed changes
src/components/legend/defaults.js
trace._module &&
trace._module.attributes &&
trace._module.attributes.showlegend &&
trace._module.attributes.showlegend.dflt === false
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain why we need this extra piece of logic:

!(
                trace._module &&
                trace._module.attributes &&
                trace._module.attributes.showlegend &&
                trace._module.attributes.showlegend.dflt === false

here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question.
This is to keep the baselines to what was in place before.
Otherwise if there are e.g. one scatter3d with other gl3d traces (that now support legend) the legend would show up on the graph for scatter3d.

diff-gl3d_world-cals

etpinard
etpinard reviewed Nov 26, 2019
View reviewed changes
src/components/legend/draw.js Outdated
Comment on lines 473 to 488
var opts = gd._fullLayout.legend;
var bw = gd._fullLayout.legend.borderwidth;
var opts = legendItem ?
gd._fullLayout.legend :
gd._fullLayout.legend.title;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a big fan of making computeTextDimensions work for the legend items AND the legend title.

What if you revert computeTextDimensions to the way it was and added a similar (but different enough) computeTitleDimensions?

Copy link
Contributor

@etpinard etpinard Dec 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I don't like the complexities you brought into computeTextDimensions, but I don't don't like them enough to block this PR.

Copy link
Contributor

@etpinard etpinard Dec 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

... so don't be surprised if I change the computeTextDimensions pattern in a future legend PR of mine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Please go for it.

etpinard
etpinard reviewed Nov 26, 2019
View reviewed changes
src/components/legend/style.js
@@ -84,6 +84,7 @@ module.exports = function style(s, gd) {
.enter().append('g')
.classed('legendpoints', true);
})
.each(styleSpatial)
Copy link
Contributor

Choose a reason for hiding this comment

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

[Do not address this comment in this PR - I'm just writing down ideas for future development]

I think legend/style.js deserves a refactor. Instead of having the styling logic for all the traces that support legends in here, we should implement a new trace module method named e.g. _module.legendItem. This would be more inline with how colorbar styling works. legend/style.js would then be pretty trivial: a few legend-wide .attr() and then a loop over the items for-each calling their corresponding _module.legendItem method. Moreover, by setting a _module.legendItem method for all the traces that support legend items, we wouldn't need the 'showLegend' trace module category.

etpinard
etpinard reviewed Nov 26, 2019
View reviewed changes
src/components/legend/style.js
case 'histogram2d' :
case 'heatmap' :
ptsData = [
['M-15,-2V4H15V-2Z'] // similar to contour
Copy link
Contributor

@etpinard etpinard Nov 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Ha you went ahead and made the heatmap legend items span rectangles:

image

instead of squares. I like it, but it might be nice to get a few +1s from other folks.

etpinard
etpinard reviewed Nov 26, 2019
View reviewed changes
src/plots/plots.js Outdated Show resolved Hide resolved
etpinard
etpinard reviewed Nov 26, 2019
View reviewed changes
src/components/legend/draw.js Outdated
.call(Drawing.font, title.font)
.text(title.text);

textLayout(titleEl, legend, gd); // handle mathjax or multi-line text and compute title height
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!! Looks like you got MathJax to work:

image

etpinard
etpinard reviewed Nov 26, 2019
View reviewed changes
test/image/mocks/geo_choropleth-legend.json
"name": "Canada",
"locations": ["CAN"],
"z": [1],
"colorscale": [[0, "blue"], [1, "blue"]],
Copy link
Contributor

Choose a reason for hiding this comment

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

cc #3468 and #1747

@etpinard
Copy link
Contributor

Thanks @archmoj for taking this on!! Looks like you're not too far off from ✅ the two items 🚀

I noticed that you didn't add any {editable: true} logic for legend titles. Is that correct? I personally don't think that feature is necessary in this iteration, but we should confirm.

After this PR, if I counted right, the trace types that have legend support are:

bar, barpolar, box, candlestick, choropleth, choroplethmapbox, cone, contourcarpet contour, densitymapbox, funnelarea, funnel, heatmap, histogram2dcontour, histogram2d, histogram, isosurface, mesh3d, ohlc, pie, pointcloud, scatter3d, scattercarpet, scattergeo, scattergl, scatter, scattermapbox, scatterpolargl, scatterpolar, scatterternary, splom, streamtube, surface, violin, volume and waterfall

and the trace types that do not have legend support are:

carpet, contourgl, heatmapgl, image, indicator, parcats, parcoords, sankey, sunburst, table and treemap

@nicolaskruchten
Copy link
Contributor

This all looks great to me! The list of traces without legend support matches my expectations!

Notes:

  1. Editable title support is nice to have if it's a low-risk afternoon of work, otherwise we'll do it later
  2. I'm fine with the heatmap legend entries being long rectangles. I'm happiest if the choropleth ones are squarer but I could live with it if we wanted them to be the same as heatmap ;)

@archmoj
RE: etienne review
a9759a2 
- stash _titleWidth and _titleHeight once in _fullLayout.legend
- bring back test for prune unsupported global-level test and use parcoords instead of choropleth
- reuse getGradientDirection function to handle reversescale case
- reserve new lines for Lib.coerce argumnets not ternary operator
- add a jasmine test for legend.title.side relayout
@archmoj
expand legend area to include long titles
b3765b4
@archmoj
add test for legends with title and scroll - fix title scroll
ce35947
@archmoj
use circle for densitymapbox legend with radial gradient
0a437a6
@nicolaskruchten
Copy link
Contributor

Do all the traces that now have legend entries therefore get legendgroup attributes also?

@etpinard
Copy link
Contributor

Do all the traces that now have legend entries therefore get legendgroup attributes also?

Yes.

@etpinard
Copy link
Contributor

etpinard commented Dec 2, 2019

Nicely done @archmoj !!

💃 💃

@archmoj archmoj merged commit 02dabc7 into master Dec 2, 2019
@archmoj archmoj deleted the new-legends-and-add-title branch December 2, 2019 16:34
This was referenced Dec 23, 2019
Closed
Merged
etpinard added a commit that referenced this pull request Dec 24, 2019
@etpinard
Merge pull request #4441 from plotly/fix-export-legend-gradients-pr4386
4137ed2 
Fix image exports for new #4386 mocks with SVG gradients
@jdamiba jdamiba mentioned this pull request Jan 9, 2020
Closed
@snyk-bot snyk-bot mentioned this pull request Feb 4, 2020
Merged
@snyk-bot snyk-bot mentioned this pull request Feb 22, 2020
Open
@snyk-bot snyk-bot mentioned this pull request Mar 18, 2020
Open
@snyk-bot snyk-bot mentioned this pull request Apr 17, 2020
Open
@ahertel ahertel mentioned this pull request May 11, 2020
Closed
@asadow asadow mentioned this pull request May 29, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@etpinard etpinard etpinard left review comments

Assignees
No one assigned
Labels
feature something new
Projects
None yet
Milestone
v1.52.0
3 participants
@archmoj @etpinard @nicolaskruchten