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: support arrays in color schemes #9262

Merged
merged 1 commit into from
Feb 17, 2024
Merged

fix: support arrays in color schemes #9262

merged 1 commit into from
Feb 17, 2024

Conversation

domoritz
Copy link
Member

fixes #9022

@domoritz domoritz requested a review from a team as a code owner February 15, 2024 12:03
@domoritz domoritz enabled auto-merge (squash) February 15, 2024 12:05
Copy link
Member

@hydrosquall hydrosquall left a comment

Choose a reason for hiding this comment

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

Thanks for making the change, I noticed this issue before but didn't know if perhaps this shorthand for setting scale.range wasn't meant to be supported. I didn't run this locally but it looks like this should work.

For future PRs - what does the workflow look like for testing changes like this? One thought is that launching a preview build of the Vega editor using this branch's version of the library could help with QA so we can validate that specs like this are schema checked happily (e.g. no yellow squiggle under the array in the JSON.).

image

@@ -611,7 +611,7 @@ export interface Scale<ES extends ExprRef | SignalRef = ExprRef | SignalRef> {
*
* For the full list of supported schemes, please refer to the [Vega Scheme](https://vega.github.io/vega/docs/schemes/#reference) reference.
Copy link
Member

Choose a reason for hiding this comment

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

Should we advertise this capability in the docs and/or a docstring?

https://vega.github.io/vega-lite/docs/scale.html#2-setting-the-range-property-to-an-array-of-valid-css-color-strings seems close but it targets scale.range rather advertising allowing scheme to be immediately set as an array. I'm assuming this arrays is a faster shorthand for the same behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I think we should revert this change since there ideally aren't multiple ways to do the same thing.

Copy link
Member Author

Choose a reason for hiding this comment

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

I sent #9266

@@ -21928,6 +21928,12 @@
{
"$ref": "#/definitions/ColorScheme"
},
{
Copy link
Member

Choose a reason for hiding this comment

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

Confirmed that this matches what we see in the underlying Vega types for RangeScheme 👍

https://github.com/vega/vega/blob/b8353d57e4842c88b81fbcfbfd7ccf48b867ae2b/packages/vega-typings/types/spec/scale.d.ts#L21

@domoritz domoritz merged commit d86c90e into main Feb 17, 2024
9 checks passed
@domoritz domoritz deleted the dom/scheme-array branch February 17, 2024 17:36
@domoritz
Copy link
Member Author

For future PRs - what does the workflow look like for testing changes like this? One thought is that launching a preview build of the Vega editor.

Right now, local checkout is needed. A preview of the editor would be awesome (similarly a preview of the website).

@hydrosquall
Copy link
Member

local checkout is needed. A preview of the editor would be awesome

I've made an issue for looking into this, I'll see if we can do this without any 3rd party providers: #9276

domoritz added a commit that referenced this pull request Mar 19, 2024
…#9262 (#9266)

reverts #9262
fixes #9022

These two seem to be equivalent so let's officially support only the
simpler one.

<img width="353" alt="Screenshot 2024-02-18 at 10 27 34"
src="https://github.com/vega/vega-lite/assets/589034/36ec606c-2fbd-4777-9442-c13536ecc8bd">

---------

Co-authored-by: GitHub Actions Bot <[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.

Support array of values for scheme
2 participants