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.

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

fix symbol set hint when fill is a constant currentColor #1830

Merged
merged 2 commits into from
Aug 24, 2023

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Aug 24, 2023

when we specify fill: "currentColor", this.fill is undefined, but we still want to pass "currentColor" as a hint to the symbol scale

closes #1462

…still want to pass "currentColor" as a hint to the symbol scale

closes #1462
@Fil Fil requested a review from mbostock August 24, 2023 09:56
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.

Shouldn’t our handling of fill and stroke be the same?

@Fil
Copy link
Contributor Author

Fil commented Aug 24, 2023

My understanding is that fill defaults to "none", and "currentColor" for fill is transformed into null in style.js because that's the SVG default for path fill.

mark.fill = impliedString(cfill, "currentColor");

but for the hint, we want to actually send an explicit fill: "currentColor" as the hint that this symbol has a fill.

The logic is different for stroke, which defaults to currentColor which is not transformed into null.

@mbostock
Copy link
Member

The logic is different for stroke, which defaults to currentColor which is not transformed into null.

Right. But I think it’s still asymmetric. In essence, we’re trying to materialize the global default fill or stroke into the hint: if there isn’t a fill channel, nor this.fill, then we’re using the implied default fill which is currentColor. And for stroke, the implied default is none. So the logic should instead be:

      symbolChannel.hint = {
        fill: fillChannel
          ? fillChannel.value === symbolChannel.value
            ? "color"
            : "currentColor"
          : this.fill ?? "currentColor",
        stroke: strokeChannel
          ? strokeChannel.value === symbolChannel.value
            ? "color"
            : "currentColor"
          : this.stroke ?? "none"
      };

@mbostock mbostock enabled auto-merge (squash) August 24, 2023 16:36
@mbostock mbostock merged commit 111cdf7 into main Aug 24, 2023
@mbostock mbostock deleted the fil/fix-symbol-set branch August 24, 2023 16:38
chaichontat pushed a commit to chaichontat/plot that referenced this pull request Jan 14, 2024
…q#1830)

* when we specify fill: "currentColor", this.fill is undefined, but we still want to pass "currentColor" as a hint to the symbol scale

closes observablehq#1462

* materialize default stroke, too

---------

Co-authored-by: Mike Bostock <[email protected]>
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.

The correct default symbol set is only inferred when fill is a channel, not a constant
2 participants