Skip to content

legends - 2 #583

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

Merged
merged 74 commits into from
Nov 29, 2021
Merged

legends - 2 #583

merged 74 commits into from
Nov 29, 2021

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Oct 26, 2021

Continuation of #539 ; build at https://observablehq.com/d/58e2c0596402e08f

In this PR the legends are [EDIT: were initially] implemented as

Plot.legend({color: mycolor, width: 640})

where mycolor is a plot or a plot scale obtained with plot.scale("color").

The same goes with {opacity:} or {r:}.

I tend to find this API less usable than the previous attempt, in the sense that:

  • we can't just say "legend: true" in the color scale's definition and have a color legend embedded in the chart
  • we can't override the function used to draw the legend
  • it's good to be able to pass options here, but frustrating that we cannot pass them in the scale definition directly.
  • passing plot.scale("color") seems redundant

UPDATE: they are now implemented as

chart.legend("color", {width: 640})

The legends themselves have been adapted from d3's color-legend notebook (with the ramp or swatches); the opacity legend is a simple greyscale ramp; the radius legend is a bunch of based circles.

(It's also very possible that not every type of scale works—there will be a lot of variations to test.)

TODO:

  • rebase on main (needs to embed the swatches css in the legend)
  • a bug that can be fixed quickly: the scale's label isn't used
  • unique css selector for swatches
  • accept a label for swatches?
  • remove Plot.legend({color: chart}) and replace with chart.legend("color", {})
  • test all types of color scales
  • remove radius legend, opacity legend
    - remove the scoped css from swatches?

LATER

Fixes #23.

@mbostock
Copy link
Member

where mycolor is a plot or a plot scale obtained with plot.scale("color")

Idea: Plot.legend({color: scale}) always takes a scale, with chart.legend("color", [legendOptions]) available as shorthand?

we can't just say "legend: true" in the color scale's definition and have a color legend embedded in the chart

Is that a requirement of this new design? Or just something we haven’t implemented yet?

@Fil
Copy link
Contributor Author

Fil commented Oct 27, 2021

I'd like to be allowed to pass any option for the legend in the scale definition.

For example, I'd like to say:

chart = Plot.plot({……… color: { swatchWidth: 40, swatchHeight: 20 } })
chart.legend("color")

However since we control with an explicit include list the parameters that get out of chart.scale("color"), this means that chart.scale should know the exact list of parameters that are allowed — this is good in one sense (a strict output), but poor in terms of separation of concerns.

It's not a blocker: we can write this list for the internally defined legends; and when a user designs a legend that takes in a new option, they can read it from the second argument to the legend("color", options) function. But it seems a bit inconsistent.

@Fil Fil mentioned this pull request Oct 27, 2021
@Fil Fil requested a review from mbostock October 29, 2021 14:13
@Fil Fil marked this pull request as ready for review October 29, 2021 14:13
@Fil Fil mentioned this pull request Nov 2, 2021
@mbostock
Copy link
Member

mbostock commented Nov 24, 2021

Looks like asymmetric diverging scales aren’t rendered correctly.

Screen Shot 2021-11-24 at 2 07 23 PM

Plot.legend({
  color: {
    type: "diverging",
    symmetric: false,
    domain: [1, 4],
    pivot: 3,
    scheme: "PiYG"
  }
})

It seems to be caused by this, where color is already an “exposed” scale from an existing plot. Which suggests that calling exposeScale or Scale is not idempotent. 🤔

exposeScale(Scale("color", undefined, color))

Ah, yes. It’s because the exposed scale drops the symmetric: false, so the symmetric transform gets applied when it shouldn’t. An easy fix for this is to always return symmetric: false on exposed diverging scales because we never want to apply the transform twice; however, that might be misleading where people think their diverging scale is asymmetric but it’s actually symmetric.

@mbostock
Copy link
Member

I’m also noticing that options on the color scale aren’t passed through to the legend in the plot.legend("color") case, which is because plot.scale("color") doesn’t return those options on the exposed scale. I’d guess that we should expose the axis/legend options on the exposed scale, though? Because if you’re sharing a scale across charts, I think you’d expect the legends to be the same, too, at least by default.

@mbostock
Copy link
Member

It looks like sqrt transforms (and possibly others) aren’t rendered correctly: the sqrt transform should apply to the tick positions, not the interpolator, but it seems to be applying to both. For example, here’s a sqrt color scale:

Screen Shot 2021-11-24 at 2 21 31 PM

Plot.plot({
  color: {
    domain: [0, 10],
    type: "sqrt"
  },
  marks: [
    Plot.cellX(d3.range(10))
  ]
})

Note that the value 2 is bright green. The legend for this suggests it should be orange:

Screen Shot 2021-11-24 at 2 24 13 PM

Plot.plot({color: {type: "sqrt", domain: [0, 10]}}).legend("color")

src/legends.js Outdated

export function legend({color, ...options}) {
if (color != null) {
return legendColor(exposeScale(Scale("color", undefined, color)), {
Copy link
Member

Choose a reason for hiding this comment

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

This will mutate color.type. 😬

Copy link
Member

@mbostock mbostock left a comment

Choose a reason for hiding this comment

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

I made a few changes 😉 but this looks good to me now! To summarize:

  • The legend implementations now take a scale descriptor (the internal representation), rather than the external representation returned by plot.scale(…). In the case where you call Plot.legend directly, the external representation is promoted to the internal one.
  • When plot.legend(…) is used, the label, ticks, and tickFormat scale options are propagated to the legend.
  • I made the legend styles consistent with Plot (adopting tabular-nums and system-ui), and adopted the same strategy of an embedded style element to allow inline styles to take precedence.
  • I rewrote a good part of the legend code; mostly to take advantage of Plot’s stricter scale definitions and options, and to clean up the swatches implementation.
  • I fixed the exposed representation of diverging scales to include symmetric: false so that the symmetric transform isn’t erroneously re-applied.
  • I removed the “normalize the options” code in Plot. The normalized options weren’t used anywhere, and I want the non-normalized options available as legend options.
  • I avoided the mutation of options.type in Scale.
  • I cleaned up the internal representation of diverging scales (and niced scales) so that the domain is always available.
  • I split the color legend tests into separate snapshots which made it easier for me to debug and review changes to the snapshots. Even just having names is helpful to describe what the test is trying to do (though comments are always welcome).

@mbostock mbostock mentioned this pull request Nov 26, 2021
7 tasks
@Fil
Copy link
Contributor Author

Fil commented Nov 29, 2021

YES

@Fil Fil merged commit bc303d5 into main Nov 29, 2021
@Fil Fil deleted the fil/legends-2 branch November 29, 2021 14:20
@Fil Fil mentioned this pull request Jan 12, 2022
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Color legend.
2 participants