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: make tick support relative band size (so it works better with binned fields) #9176

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

kanitw
Copy link
Member

@kanitw kanitw commented Nov 15, 2023

Please review and merge #9178 first (since it will fix one of the bad screenshot diffs).
(After merging, #9178 into this PR, we also fixed 2aeffca.)

Context: Bar/Rect marks use encode.rectPosition, which has sophisticated logic to determine x+width, or y+height.
However, tick previously only use encode.pointPosition to determine X & Y and encode.nonPosition and localized defaultSize to determine width or height depending on orientation.

With this PR, we make tick's width dynamic to the bin too.

image

Also, this is a prerequisite to make tick support relative band width

This PR replaces local defaultSize calculate to make bar share the rules for its dimension axis with bar/rect and thus allow ticks to better adapt to binned fields.

cc: @richardliu-db

kanitw and others added 3 commits November 15, 2023 15:32
(by sharing the code for tick's dimension axis with rectangle/bar)

Also convert `RectBinSpacingMixins` into `BandSizeConfigMixins`
Copy link
Member Author

Choose a reason for hiding this comment

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

This file has the core change to use positionRect with tick instead.

@@ -77,7 +77,7 @@ function defaultType(
}

if (isXorY(channel) || isXorYOffset(channel)) {
if (util.contains(['rect', 'bar', 'image', 'rule'], mark.type)) {
if (util.contains(['rect', 'bar', 'image', 'tick', 'rule'], mark.type)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Need to make tick use band scale by default, just like bar/rect, to leverage the same set of logic.

@@ -284,17 +284,6 @@ export interface MarkConfig<ES extends ExprRef | SignalRef>
outerRadius?: number | ES;
}

export interface RectBinSpacingMixins {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is blended with BandSizeConfigMixins below

@kanitw kanitw changed the title fix: better adapt tick width dimension with binned fields fix: make tick support relative band size (so it works better with binned fields) Nov 16, 2023
line: ['point'],
tick: ['bandSize', 'thickness']
tick: ['bandSize', 'thickness', ...VL_ONLY_BAND_SIZE_CONFIG_MIXINS_PROPS]
Copy link
Member Author

Choose a reason for hiding this comment

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

FYI, we need to add this, so we don't output these properties in the output Vega config.

export interface TickConfig<ES extends ExprRef | SignalRef>
extends MarkConfig<ES>,
TickThicknessMixins,
BandSizeConfigMixins<ES> {
Copy link
Member Author

Choose a reason for hiding this comment

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

Tick now has BandSizeConfigMixins, because we're making tick more adaptable to band + support relative band size, by using rectPosition.

@@ -678,7 +690,10 @@ export interface TickConfig<ES extends ExprRef | SignalRef> extends MarkConfig<E
}

export const defaultTickConfig: TickConfig<SignalRef> = {
thickness: 1
thickness: 1,
discreteBandSize: {band: 0.75},
Copy link
Member Author

Choose a reason for hiding this comment

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

image

0.75 here are mainly to match 3/4 multiplier in the old code:

image

Copy link
Contributor

Choose a reason for hiding this comment

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

The two screenshots look the same to me.

"name": "height",
"update": "bandspace(domain('y').length, 1, 0.5) * y_step"
}
{"name": "height", "update": "bandspace(domain('y').length, 0, 0) * y_step"}
Copy link
Member Author

Choose a reason for hiding this comment

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

This diff is coming from switching scale types.

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.

2 participants