Skip to content
This repository has been archived by the owner on Dec 10, 2021. It is now read-only.

feat: add functions for parsing scales #207

Merged
merged 23 commits into from
Aug 28, 2019
Merged

feat: add functions for parsing scales #207

merged 23 commits into from
Aug 28, 2019

Conversation

kristw
Copy link
Contributor

@kristw kristw commented Aug 21, 2019

🏆 Enhancements

This is part of the ongoing work for @superset-ui/encodeable.

  • Add types related to scales. Inherit types from vega-lite and override some fields.
  • Add type guards
  • Add functions for parsing scales from configuration object into D3 scales as well as connecting to @superset-ui/colors to get color schemes by name. Support most types of scale that D3/vega-lite has. See [vega-lite's scale documentation](https://vega.github.io/vega-lite/docs/scale.html for more details).
type ScaleConfig<Output extends Value = Value> =
  | LinearScaleConfig<Output>
  | LogScaleConfig<Output>
  | PowScaleConfig<Output>
  | SqrtScaleConfig<Output>
  | SymlogScaleConfig<Output>
  | TimeScaleConfig<Output>
  | UtcScaleConfig<Output>
  | QuantileScaleConfig<Output>
  | QuantizeScaleConfig<Output>
  | ThresholdScaleConfig<Output>
  | BinOrdinalScaleConfig<Output>
  | OrdinalScaleConfig<Output>
  | PointScaleConfig<Output>
  | BandScaleConfig<Output>;
  • Add tons of unit tests to keep 100% coverage.

The main function in this PR is createScaleFromScaleConfig(...)

Example usage

const scale = createScaleFromScaleConfig({
  type: 'linear',
  domain: [0, 10],
  range: [0, 100],
});
expect(scale(10)).toEqual(100);
const scale = createScaleFromScaleConfig({
  type: 'utc',
  domain: [
    {
      year: 2019,
      month: 7,
      date: 5,
      utc: true,
    },
    {
      year: 2019,
      month: 7,
      date: 30,
      utc: true,
    },
  ],
  range: [0, 100],
  nice: { interval: 'month', step: 2 },
});
expect((scale as ScaleTime<number, number>).domain()).toEqual([
  new Date(Date.UTC(2019, 6, 1)),
  new Date(Date.UTC(2019, 8, 1)),
]);

The following scale properties are supported. (See Scale.ts.)

'domain'
'range'
'reverse'
'align'
'base'
'clamp'
'constant'
'exponent'
'interpolate' // add placeholder to be implemented
'nice'
'padding'
'paddingInner'
'paddingOuter'
'reverse'
'round'
'scheme'
'namespace'
'zero'

@kristw kristw requested a review from a team as a code owner August 21, 2019 00:07
@kristw kristw changed the title [WIP] Add functions for parsing scales [WIP] feat: add functions for parsing scales Aug 21, 2019
@codecov
Copy link

codecov bot commented Aug 21, 2019

Codecov Report

Merging #207 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #207      +/-   ##
==========================================
+ Coverage   99.91%   99.92%   +<.01%     
==========================================
  Files         112      130      +18     
  Lines        1245     1406     +161     
  Branches      307      354      +47     
==========================================
+ Hits         1244     1405     +161     
  Partials        1        1
Impacted Files Coverage Δ
...i-encodeable/src/parsers/scale/applyInterpolate.ts 100% <100%> (ø)
...set-ui-encodeable/src/parsers/scale/applyDomain.ts 100% <100%> (ø)
...rset-ui-encodeable/src/parsers/scale/applyClamp.ts 100% <100%> (ø)
...le/src/parsers/scale/createScaleFromScaleConfig.ts 100% <100%> (ø)
...es/superset-ui-dimension/src/svg/updateTextNode.ts 100% <100%> (ø) ⬆️
...src/parsers/scale/getScaleCategoryFromScaleType.ts 100% <100%> (ø)
...-ui-encodeable/src/parsers/scale/inferScaleType.ts 100% <100%> (ø)
...erset-ui-encodeable/src/parsers/scale/applyZero.ts 100% <100%> (ø)
...rset-ui-encodeable/src/parsers/scale/applyRound.ts 100% <100%> (ø)
...rc/parsers/scale/isPropertySupportedByScaleType.ts 100% <100%> (ø)
... and 27 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8baede4...b40089d. Read the comment docs.

@netlify
Copy link

netlify bot commented Aug 21, 2019

Deploy preview for superset-ui ready!

Built with commit b40089d

https://deploy-preview-207--superset-ui.netlify.com

@kristw kristw force-pushed the kristw--encode-scale branch from 0460162 to f027bcb Compare August 27, 2019 23:17
@kristw kristw changed the title [WIP] feat: add functions for parsing scales feat: add functions for parsing scales Aug 28, 2019
@kristw kristw added #enhancement New feature or request reviewable Ready for review labels Aug 28, 2019
@schillerk
Copy link

Nice! Looks great, I wish there was a better abstraction for property in obj && typeof obj.property !== 'undefined', but I guess that's just Javascript.

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.

this is a ton of work and looks great overall! didn't have much to add ✨

.replace(/\)$/, '')
.split(',')
.map((chunk: string) => Number(chunk.trim())) as [
number,
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 it'd be useful to add comments for the parts, can probably just look up the Date api

// if (isAFunction(scaleFn)) const encodedValue = scaleFn(10)
//
// CategoricalColorScale is actually a function,
// but TypeScript is not smart enough to realize that by itself.
Copy link
Contributor

Choose a reason for hiding this comment

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

yeah this is a tricky one

config: ScaleConfig<Output>,
) {
switch (config.type) {
default:
Copy link
Contributor

Choose a reason for hiding this comment

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

whoa have never seen default first 🤯

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will move to bottom :)

// quantile depends on distribution so zero does not matter
zeroSet.delete(ScaleType.QUANTILE);

const supportedScaleTypes: Record<keyof CombinedScaleConfig, Set<ScaleType>> = {
Copy link
Contributor

Choose a reason for hiding this comment

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

nice 💯

export const continuousScaleTypes: ScaleType[] = continuousToContinuousScaleTypes;
export const continuousScaleTypesSet = continuousToContinuousScaleTypesSet;

export const discreteScaleTypes: ScaleType[] = ['band', 'point', 'ordinal'];
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason not to use ScaleType.BAND, etc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope. forgot changed. Thank you for catching.


export default function getScaleCategoryFromScaleType(scaleType: ScaleType) {
if (continuousScaleTypesSet.has(scaleType)) {
return 'continuous';
Copy link
Contributor

Choose a reason for hiding this comment

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

do you think it's worth making these constants? or could at least specify the return type is ScaleCategory | undefined?

Copy link
Contributor Author

@kristw kristw Aug 28, 2019

Choose a reason for hiding this comment

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

agree 👍 . Will specify the return type to enforce stricter check

@kristw kristw merged commit 6f56ed9 into master Aug 28, 2019
@delete-merged-branch delete-merged-branch bot deleted the kristw--encode-scale branch August 28, 2019 19:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
#enhancement New feature or request reviewable Ready for review size/XXL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants