Skip to content

include LegendOptions (e.g., className, fontSize, r…) in the type for scales#2175

Open
Fil wants to merge 1 commit intomainfrom
fil/fix-legend-type
Open

include LegendOptions (e.g., className, fontSize, r…) in the type for scales#2175
Fil wants to merge 1 commit intomainfrom
fil/fix-legend-type

Conversation

@Fil
Copy link
Copy Markdown
Contributor

@Fil Fil commented Sep 24, 2024

No description provided.

@Fil Fil requested a review from mbostock September 24, 2024 09:47
Copy link
Copy Markdown
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.

This seems right, but it also seems a little dangerous to mix the legend options into the scale options. Maybe we could do it more like the tip mark option, where instead of legend: true you could pass legend: options?

Plot.plot({
  color: {
    type: "ordinal",
    scheme: "rainbow",
    legend: {
      className: "legend",
      style: {fontSize: "16px"}
    }
  },
  style: {fontSize: "7px"},
  marks: [Plot.cell("ABCDEFGHIJ", {x: Plot.identity, fill: Plot.identity})]
})

We would then change ScaleDefaults from:

legend?: LegendOptions["legend"] | boolean | null;

to:

legend?: LegendOptions | LegendOptions["legend"] | boolean | null;

It’d probably be safe to just make this change, but we could also be conservative and continue supporting the undocumented way of mixing legend options into scale options if desired.

Copy link
Copy Markdown
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.

This was also superseded by #2249, which included type declarations for legend options within scale options. Note that we only want legend options for scales that can have legends, which is currently the color, opacity, and symbol scales; if other scales support legends in the future (such as r #236) then we can add those types as appropriate then.

There does seem to be a bug in the current implementation in that ColorLegendOptions only includes the options that are specific to color legends and does not include the generic LegendOptions. That’s because LegendOptions extends ColorLegendOptions rather than the other way around.

Fil added a commit that referenced this pull request Apr 13, 2026
Generic legend options (`label`, `tickFormat`, `fontVariant`, `style`, `className`) were only declared on LegendOptions, which extends ColorLegendOptions. This meant passing `style` or `className` inside scale options was a type error.

Instead we introduce `BaseLegendOptions` and have each specific legend type extend it.

Fixes #2175
@Fil
Copy link
Copy Markdown
Contributor Author

Fil commented Apr 13, 2026

superseded by #2421

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.

2 participants