Skip to content

Common style channels #490

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 20 commits into from
Aug 9, 2021
Merged

Common style channels #490

merged 20 commits into from
Aug 9, 2021

Conversation

mbostock
Copy link
Member

@mbostock mbostock commented Aug 8, 2021

Related #322, this is a sketch at how we might consolidate the definition of “standard” channels in Plot. This idea is to define them in one place so that we can add a new standard channel to all marks with less risk of drift between marks.

This only currently works for the Area mark. In addition to porting the other marks to use this system, we need to address that some marks have different defaults (for example Line has a default stroke, whereas Area has a default fill), and that some marks have different structure (for example Area and Line produce a single path element per series and thus group styles, whereas Dot and Bar product distinct elements for each data point). I’ve done most of this now.

TODO

  • areas
  • bars
  • cells
  • dots (quirk: default fill)
  • frames (quirk: does not support channels)
  • lines
  • links
  • rects
  • rules
  • texts
  • ticks

If you like this idea I’d be happy to continue work and/or work on it together.

@mbostock mbostock requested a review from Fil August 8, 2021 17:00
@Fil
Copy link
Contributor

Fil commented Aug 9, 2021

Yes that's a big relief, and besides making Plot smaller and more consistent, it will also make custom marks much easier to write.

I stopped short of:

  • adding the channels x, x1, x2, y, y1, y2 in the default filter (and then maybe also r and text?)
  • renaming filter to definedFilter, which would be clearer about the purpose

@Fil
Copy link
Contributor

Fil commented Aug 9, 2021

And #322 became so much simpler!

@Fil Fil marked this pull request as ready for review August 9, 2021 10:26
@Fil
Copy link
Contributor

Fil commented Aug 9, 2021

Wait, I forgot to apply this in the facet's call to mark.render

@Fil Fil mentioned this pull request Aug 9, 2021
@mbostock
Copy link
Member Author

mbostock commented Aug 9, 2021

Here’s a quirk that I’m thinking about currently.

Some marks default to a fill (with no stroke), while others default to a stroke (with no fill). For example, bar and area default to a fill, while dot and line default to a stroke. (Also some marks don’t support a fill, such as ticks and rules, but for now we can treat that as the same as marks that default a stroke.)

But that’s not all. For marks that have a default fill (such as bar and area), the default fill only applies if the stroke is (constant) none; if you set a stroke, then the default fill switches to none. Similarly for marks that have a default stroke (such as dot), the default stroke only applies if the fill is (constant) none. But line is not consistent with this: if you apply a fill, it retains the default stroke.

So, do we want to standardize on the behavior of dot which is symmetrical, or do we want to standardize on the behavior of line which is (arguably) simpler? And in any case if we make dot and line behave the same way, it’s not strictly backwards-compatible. 🤔

@mbostock
Copy link
Member Author

mbostock commented Aug 9, 2021

I vote that we standardize on the symmetrical behavior (dot’s), and therefore change the behavior for line. Setting a fill on line is very rare anyway so I think it’s unlikely to cause trouble if the default stroke goes away when you set a fill.

@mbostock mbostock merged commit b1885ad into main Aug 9, 2021
@mbostock mbostock deleted the mbostock/common-styles branch August 9, 2021 19:32
@Fil Fil mentioned this pull request Jul 20, 2022
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