Skip to content
This repository has been archived by the owner on Jun 25, 2020. It is now read-only.

feat: add encodeable utilities for chart #15

Merged
merged 9 commits into from
Mar 20, 2019
Merged

Conversation

kristw
Copy link
Collaborator

@kristw kristw commented Mar 13, 2019

🏆 Enhancements

Define encoding config for a visualization

  • Each visualization has one or more channels that can be encoded, such as x, y, color, stroke, fill. These channels configurations should be consistent/sharable across all visualizations. For example, x and y usually have scales and axes. Scales have domain, range. Axes have label, format. Color can support categorical, sequential or fixed color. All channels can support fixed value, field name in the data, or sometimes aggregates (haven't implemented the aggregate yet).
  • Each encoding channel config is based on vega-lite encoding grammar. (See line chart example, vega-lite encoding documentation)
  • Only adopt a small subset of vega-lite encoding grammar for now as we don’t have enough time (and may not intend) to provide 100% feature of vega-lite.

Implement TypeScript types for the encoding config.

  • First attempt was using all types from vega-lite package (XXXFieldDef) directly but ended up getting too overwhelmed.
  • Now adopting the primitive types from vega-lite then write higher-level types that has same name but only with the subset of the fields, so we can keep that of what we support and slowly adopt more in the future instead of taking it all now which our encoder does not know how to handle and also make it harder to implement the encoder. (preview)

Write an encoder that parses the encoding config mention above and become a helper during visualization rendering.

Long shot

  • Once all these encoding configs code are a bit more stable, it could be extracted to @superset-ui/encodeable.

@kristw kristw requested a review from a team as a code owner March 13, 2019 18:00
@kristw kristw added #enhancement New feature or request reviewable labels Mar 13, 2019
@kristw kristw force-pushed the kristw--encodable branch from adf8069 to be15fbd Compare March 14, 2019 01:50

export interface Dataset {
keys: string[];
values: PlainObject[];
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if we can make sure that the key in the PlainObject will be always in the set of key? seems impossible.\

Copy link
Contributor

Choose a reason for hiding this comment

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

I googled a bit and couldn't figure this out, worth asking in #typescript tho?

Copy link
Collaborator Author

@kristw kristw Mar 14, 2019

Choose a reason for hiding this comment

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

Can use typeguard to help. Not sure if possible to define solely as type without knowing the fields before hand.

export type PlainObject<Key extends string = string, Value extends any = any> = {
  [key in Key]: Value
};

export type Dataset<T extends string> = {
  keys: T[];
  values: Partial<PlainObject<T>>[];
}

function isDataset<T extends string>(input: Dataset<T>): Dataset<T> {
  return input;
}

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

image

Copy link
Collaborator Author

@kristw kristw Mar 14, 2019

Choose a reason for hiding this comment

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

mmm. this is more complicated. the guard only work for inline input, but if you pass a variable with type already it make keys string[] instead of 'a'|'b'|'c'[] and the check does not work.

Copy link
Contributor

@williaster williaster left a comment

Choose a reason for hiding this comment

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

Looking great overall! some crazy TS in there 😎

Thanks for the hard work on this!

x: number | null;
y: number | null;
color: string;
fill: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

does a boolean mean it impossible to have different color / fill?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Umm, at this point yes. the fill is gradient from [stroke] color to white.
I can make this boolean | string, which may lead to some ugly line charts with stroke and fill not getting along.


export interface Dataset {
keys: string[];
values: PlainObject[];
Copy link
Contributor

Choose a reason for hiding this comment

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

I googled a bit and couldn't figure this out, worth asking in #typescript tho?

@kristw kristw force-pushed the kristw--encodable branch from be15fbd to 9c3c907 Compare March 14, 2019 23:50
@kristw
Copy link
Collaborator Author

kristw commented Mar 15, 2019

Addressed most of the comments.

Copy link
Contributor

@williaster williaster left a comment

Choose a reason for hiding this comment

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

Overall this looks super clean and solid improvements vs the first round! 😍 🎉 I think none of my comments are blocking and that we could extend the fill option to accept a string in the future if need be.

thanks for all the hard work on this, pretty crazy TS here! ❤️ will approve from my end but still let @conglei comment/review if he wants!

"lodash": "^4.17.11",
"prop-types": "^15.6.2",
"reselect" : "^4.0.0",
"vega": "^5.2.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't see any imports of vega, is this because it's a vega-lite dep? bundle size should be okay with the deep imports I'd think.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Tried removing it and realize vega-lite needs it.

Copy link
Contributor

@conglei conglei 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 adding this! 🥂 It looks good to me!

@kristw kristw force-pushed the kristw--encodable branch from 6f03d9f to 6825b18 Compare March 20, 2019 20:34
@kristw kristw merged commit 287d37e into master Mar 20, 2019
@delete-merged-branch delete-merged-branch bot deleted the kristw--encodable branch March 20, 2019 22:23
nytai referenced this pull request in preset-io/superset-ui-plugins Apr 27, 2020
nytai referenced this pull request in preset-io/superset-ui-plugins Apr 27, 2020
* feat: add encodeable utilities

* feat: add types back

* refactor: simplify function calls

* refactor: rename generic type

* refactor: more edits

* refactor: remove unused function

* refactor: rename file

* fix: address comments

* fix: add vega back
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
#enhancement New feature or request reviewable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants