Skip to content

extend to text and vector; document #708

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

Closed
wants to merge 5 commits into from
Closed

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Jan 26, 2022

  • Pending issue: the default is 3 for dodge, but can be 4.5 for dot if there is a symbol channel.

@Fil Fil requested a review from mbostock January 26, 2022 16:48
@Fil Fil marked this pull request as ready for review January 26, 2022 16:58
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.

It seems wrong for marks to define channels they don’t use, just so they can be made available to an unrelated layout.

In the case of a constant r, this change isn’t necessary as the dodge layout already reads the r option even if the downstream mark doesn’t. If we want do do a channel (variable) r, then we’ll need a way for layouts to be able to register extra channels. But, do you think that’s required?


### Dodge

The dodge layout can be applied to the Dot, Text and Vector marks.
Copy link
Member

@mbostock mbostock Jan 26, 2022

Choose a reason for hiding this comment

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

It can be applied to any mark (that consumes x or y).

@Fil
Copy link
Contributor Author

Fil commented Jan 26, 2022

I want to support the same layout on text marks, so we can do this:
Capture d’écran 2022-01-26 à 18 00 19

Maybe marking r optional in the mark itself is the wrong approach, and what we need instead is to allow the layout to ask for the (scaled) channels it wants to use? It would seem more appropriate in terms of separation of concerns.

We also don't want users to have to pass the r option differently—and maybe r is going to be the result of a bin transform—, so it shouldn't be a dodgeOption but part of the normal options.

@mbostock
Copy link
Member

mbostock commented Jan 26, 2022

I want to support the same layout on text marks, so we can do this

As I said, your test works as-is in the mbostock/dot-dodge branch (because the dodge layout will check for the r option even if it’s ignored by the mark). And similarly in the case of a bin transform that produces an r channel, the dodge layout will consume it.

The only thing that’s missing is that if you want an r channel (i.e., variable radius) but the downstream mark doesn’t support it, then we’d need a way for the dodge layout to register that channel somehow. Maybe we could tack on a layout.channels array or something.

This discussion also makes me thing we want a way to alias channels that were computed by other marks. Like, if you dodge dots, it’s not great that we need to redo all the work to dodge text labels on top of the dots; ideally you want a way to derive the channels for the text mark from the previously-computed channels for the dot mark. Maybe we can figure out a way to do that?

@Fil
Copy link
Contributor Author

Fil commented Jan 26, 2022

if you want an r channel (i.e., variable radius) but the downstream mark doesn’t support it

Yes, this is what I was aiming for with these changes (the test plot I added is simpler, and it already works). I like the idea of adding a layout.channels.

The default of 3 vs 4.5 for symbols is not a huge issue, even though it would be nicer if we respected the same defaults (tracked in #711)

I realize I forgot to mention images. Beyond that, I haven't been able to make it work with any other mark yet, like ticks, rules, lines… not that it needs to be working, but I think the documentation needs to capture where a layout can be applied.

Closing for now.

@Fil Fil closed this Jan 26, 2022
@mbostock
Copy link
Member

The default of 3 vs 4.5 for symbols is not a huge issue, even though it would be nicer if we respected the same defaults.

Yes. One possibility is that, rather than consuming the passed-in options, the layout could consult this.r when it is invoked. This way it would be able to check if the mark provides a default radius. But in the case that a mark doesn’t implement the r option, the dodge layout would presumably still need to fallback to the passed in r option.

@Fil Fil deleted the fil/dot-dodge branch January 27, 2022 18:41
@mbostock mbostock mentioned this pull request Jan 28, 2022
@Fil
Copy link
Contributor Author

Fil commented Mar 31, 2023

An example here https://observablehq.com/@recifs/dodge-text

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