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

feat: Improve color scheme type #9015

Merged
merged 8 commits into from
Jul 19, 2023

Conversation

mcnuttandrew
Copy link
Contributor

@mcnuttandrew mcnuttandrew commented Jul 18, 2023

The JSON Schema that vega-lite generates is wonderful, but there are a few places where the types are slightly under-defined because they are defined as part of the vega type system.

One such example is color schemes, which are more permissive than should be allowing any string. While the included description is helpful for identifying the allow values ("A string indicating a color [scheme](https://vega.github.io/vega-lite/docs/scale.html#scheme) name (e.g., "category10"or"blues") or a [scheme parameter object](https://vega.github.io/vega-lite/docs/scale.html#scheme-params)...."), this is not very helpful systems ingesting the schema computationally, such as for structure editing.

Fortunately, a possible workaround is to explicitly import the wonderful vega-typings package (which was already an implied dependency, cf yarn.lock). This allows use of the more explicit type described by vega-typings.

Please:

  • Make the pull requests (PRs) atomic (fix one issue at a time). Multiple relevant issues that must be fixed together? Make atomic commits so we can easily review each issue.
  • Provide a concise title as a semantic commit message (e.g. "fix: correctly handle undefined properties") so we can easily copy it to the release note.
    • Use imperative mood and present tense.
  • Mention relevant issues in the description (e.g., Fixes #1 / Fixes part of #1).
  • Lint and test (Run yarn test).
  • Review your changes before sending the PR (to ensure code quality).
  • For new features:
    • Add new unit tests.
    • Update the documentation under site/docs/ + add examples.

Tips:

package.json Outdated
@@ -127,6 +127,7 @@
"vega-datasets": "^2.7.0",
"vega-embed": "^6.22.1",
"vega-tooltip": "^0.32.0",
"vega-typings": "^0.24.1",
Copy link
Member

Choose a reason for hiding this comment

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

Since we already depend on Vega, I don’t think we need this. It just makes updates more annoying.

examples/compiled/interactive_point_domainRaw_binding.png Outdated Show resolved Hide resolved
src/scale.ts Outdated
@@ -458,7 +459,7 @@ export interface SchemeParams {
*
* For the full list of supported schemes, please refer to the [Vega Scheme](https://vega.github.io/vega/docs/schemes/#reference) reference.
*/
name: string | SignalRef;
name: string | ColorScheme | SignalRef;
Copy link
Member

Choose a reason for hiding this comment

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

This is great. Should we even allow string?

Copy link
Contributor Author

@mcnuttandrew mcnuttandrew Jul 19, 2023

Choose a reason for hiding this comment

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

Good point! I went with a more permissive type because I wasn't sure what the down stream deps were from this type, but if this seems fine to you then a more assertive type is great!

@domoritz domoritz merged commit d535812 into vega:main Jul 19, 2023
@domoritz
Copy link
Member

Thank you!

@PBI-David
Copy link
Contributor

PBI-David commented Jul 20, 2023

Will this still allow the following which I can do in Vega?


"range": {
        "scheme": [
          "#1860cc",
          "#18d2d9",
          "#18cc36"
        ]
      }

@domoritz
Copy link
Member

Screenshot 2023-07-20 at 08 47 50

We actually didn't even support that in the first place, but should!

@domoritz
Copy link
Member

I filed #9022

BradyJ27 pushed a commit to BradyJ27/vega-lite that referenced this pull request Oct 19, 2023
@domoritz
Copy link
Member

@PBI-David you can already set the range to a list: https://vega.github.io/vega-lite/docs/scale.html#2-setting-the-range-property-to-an-array-of-valid-css-color-strings

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.

3 participants