-
Notifications
You must be signed in to change notification settings - Fork 185
per-channel scale override, and “auto” scale #1247
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
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
a915648
per-channel scale override; "auto" scale
mbostock 4b6aedd
document the {value, scale} pattern for color and symbol channels
Fil d33e69b
Update README.md
Fil 8fce438
const style
mbostock 1f87e44
validate scale name
mbostock a93208a
fix inferred scale for initializers
mbostock 6e5f2ea
inferChannelScale
mbostock 023ea9f
less painful colors
mbostock ed0c049
Update README.md
mbostock ff84387
Update README.md
mbostock 1eafabf
Update README.md
mbostock fccd651
Merge branch 'fil/channel-scale-opt-out' into mbostock/channel-scale-…
mbostock b6ca953
Update README
mbostock File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we guard against bad scale values?
For example, we want to throw an error if the user passes
stroke: {value: "field", scale: "x"}
or
strokeOpacity: {value: Math.random, scale: "x"}
I was thinking we should test that
channel.scale === "auto"
, and that value.scale is one of ["color", null] if name is stroke or color, and one of ["symbol", null] if name is symbol. And error in all other cases?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You’re saying we should restrict which scales we allow to be bound to which channels? Why? Is that likely? What if someone invents a creative use for doing something we didn’t anticipate?
I do think it’d be reasonable to enforce that the scale, if non-nullish, is a valid scale name (and not, say
scale: "foo"
). But I’m not sure we should do any validation beyond that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I don't think the symbol or color channel can be assigned to x, even in a creative way—since their ranges are not in the same space? x, y, r and length are numbers, so there might be cases where you'd want to mix those, maybe—but most probably it's going to be a mistake, or a misunderstanding. (I also tried true, false… these will be fixed if we make sure that scale is nullish or a key of scales.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve added validation of the scale name when the channel is registered, but only that the scale name is known (in the scale registry). Note it was already the case that a mark could declare a channel bound to a scale that doesn’t exist; it was being treated as if the channel were not bound to a scale. Though since this PR offers users new control over how channels are bound to scales without implementing a custom mark, it seems reasonable to add some validation.
I understand that it doesn’t make sense to put
scale: "x"
on the fill channel, or some such. But I also don’t see that we need to protect against it. There are so many other ways you can break plot. Adding validation requires us to codify which scales are allowed for which channels; we haven’t yet formalized this policy, and I don’t see a strong reason to do it now. Let’s address this in the future if users actually trip on it.