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: Allow config to set default scale.zero per marktype #8354

Merged
merged 7 commits into from Aug 16, 2022

Conversation

yhoonkim
Copy link
Contributor

@yhoonkim yhoonkim commented Aug 11, 2022

#8324 Allow config to set default scale.zero per marktype.

  • config.scale.zero is true as default.
  • When config.scale.zero===false, zero is false as default except for the non-ranged bar/area chart.
πŸ“¦ Published PR as canary version: 5.5.1--canary.8354.6729853.0

✨ Test out this PR locally via:

npm install vega-lite@5.5.1--canary.8354.6729853.0
# or 
yarn add vega-lite@5.5.1--canary.8354.6729853.0

Version

Published prerelease version: v5.6.0-next.0

Changelog

πŸš€ Enhancement

Authors: 2

@yhoonkim yhoonkim force-pushed the yh/config-zero branch 4 times, most recently from a43105c to cf03378 Compare August 12, 2022 00:25
@yhoonkim yhoonkim requested a review from kanitw August 12, 2022 02:25
@yhoonkim yhoonkim marked this pull request as ready for review August 12, 2022 02:25
@@ -60,7 +60,11 @@
"domain": {"data": "data_0", "fields": ["start", "end"]},
"range": [0, {"signal": "width"}],
"nice": true,
<<<<<<< Updated upstream
Copy link
Member

@kanitw kanitw Aug 15, 2022

Choose a reason for hiding this comment

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

I know you fix this later, but you may want to leverage interactive rebase to clean this up in the future, so reviewers see less noise as they step through commits ;)


// 1) using quantitative field with size
// While this can be either ratio or interval fields, our assumption is that
// ratio are more common. However, if the scaleType is discretizing scale, we want to return
// false so that range doesn't start at zero
if (channel === 'size' && fieldDef.type === 'quantitative' && !isContinuousToDiscrete(scaleType)) {
return true;
return defaultZero;
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if config.zero also shouldn't affect size scale (since quantitative size scale are probably should scale down to zero).

@@ -120,7 +120,8 @@ function parseUnitScaleProperty(model: UnitModel, property: Exclude<keyof (Scale
domainMax: specifiedScale.domainMax,
markDef,
config,
hasNestedOffsetScale: channelHasNestedOffsetScale(encoding, channel)
hasNestedOffsetScale: channelHasNestedOffsetScale(encoding, channel),
hasRangeChannels: encodingHasRangeChannels(encoding)
Copy link
Member

Choose a reason for hiding this comment

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

The fact that encodingHasRangeChannels(encoding) doesn't take in channel seems alarming to me.

Basically, Y2 shouldn't affect x-scale's zero.

I think you should

  • undo adding encodingHasRangeChannels
  • rename hasRangeChannels to hasRangeChannel and just check if there is x2 for x or y2 for x. (And maybe radius2 for radius.)

@@ -444,7 +444,7 @@ To provide themes for all scales, the scale config (`config: {scale: {...}}`) ca

#### Padding

{% include table.html props="bandPaddingInner,barBandPaddingInner,rectBandPaddingInner,bandWithNestedOffsetPaddingInner,offsetBandPaddingInner,bandPaddingOuter,bandWithNestedOffsetPaddingOuter,offsetBandPaddingOuter,continuousPadding,pointPadding" source="ScaleConfig" %}
{% include table.html props="bandPaddingInner,barBandPaddingInner,rectBandPaddingInner,bandWithNestedOffsetPaddingInner,offsetBandPaddingInner,bandPaddingOuter,bandWithNestedOffsetPaddingOuter,offsetBandPaddingOuter,continuousPadding,pointPadding,zero" source="ScaleConfig" %}
Copy link
Member

Choose a reason for hiding this comment

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

Why is zero added to the padding table? This seems wrong?

src/scale.ts Outdated
@@ -415,6 +415,14 @@ export interface ScaleConfig<ES extends ExprRef | SignalRef> {
* Reverse x-scale by default (useful for right-to-left charts).
*/
xReverse?: boolean | ES;

/**
* Override the default `scale.zero` for [`continuous`](https://vega.github.io/vega-lite/docs/scale.html#continuous) scale except for non-ranged bar or area charts.
Copy link
Member

Choose a reason for hiding this comment

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

continuous scale

scales

Copy link
Member

Choose a reason for hiding this comment

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

except for non-ranged bar or area charts.

If you agree with the other comment to always do "zero" for size scale.

except for (1) x/y-scales of non-ranged bar or area charts and (2) size scales.

@@ -418,18 +422,21 @@ export function zero(
}
}

// If there is no custom domain, return true only for the following cases:
const defaultZero = scaleConfig?.zero === undefined ? true : scaleConfig?.zero;
Copy link
Member

Choose a reason for hiding this comment

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

Instead of adding this line, probably cleaner to add zero: true to defaultScaleConfig.

yhoonkim and others added 6 commits August 15, 2022 18:13
- Undo encodingHasRangeChannels
- Set `true` for continuous size scale regardless to config
- Fix doc
@yhoonkim yhoonkim merged commit 2f3c359 into next Aug 16, 2022
12 checks passed
@yhoonkim yhoonkim deleted the yh/config-zero branch August 16, 2022 18:25
@vega-org-bot vega-org-bot mentioned this pull request Aug 18, 2022
@vega-org-bot vega-org-bot mentioned this pull request Oct 10, 2022
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants