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 bars with one axis thicker #8894

Merged
merged 6 commits into from
May 11, 2023
Merged

Conversation

yhoonkim
Copy link
Contributor

@yhoonkim yhoonkim commented May 9, 2023

Added the logic making the bar's size thicker if there is no secondary channel.

@yhoonkim yhoonkim requested a review from kanitw May 9, 2023 23:41
@@ -110,6 +111,9 @@ function defaultSizeRef(
return {value: scaleRange.step - 2};
}
}
if (!hasFieldDef) {
return {signal: `0.8 * ${sizeChannel}`};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wonder if we extract {signal: 0.8 * ${sizeChannel}} as the constant. But I could not find a good place to place if we extract...

Copy link
Member

@kanitw kanitw May 10, 2023

Choose a reason for hiding this comment

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

Very good question. For consistency with existing similar style, we can respect config.scale.bandPaddingInner/ barBandPaddingInner/rectBandPaddingInner.

Basically, we can think of axis without a field as categorical axis with one unique value.

So we can do:

const padding = getFirstDefined(bandPaddingInner, mark === 'bar' ? barBandPaddingInner : rectBandPaddingInner); // this part is like paddingInner in scale.ts
return {signal: `${1 - padding} * ${sizeChannel}`}; 

Copy link
Member

Choose a reason for hiding this comment

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

For this example, we may want to set config.scale.barBandPaddingInner = 0

@kanitw kanitw changed the title feat: Make bars without secondary channel thicker fix: Make bars without secondary channel thicker May 10, 2023
@kanitw kanitw changed the title fix: Make bars without secondary channel thicker fix: Make bars with one axis thicker May 10, 2023
Copy link
Member

@kanitw kanitw left a comment

Choose a reason for hiding this comment

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

Last few nits before merging. :)

return {signal: `(1 - (${padding.expr})) * ${sizeChannel}`};
}
/* istanbul ignore next: Condition should not happen -- only for warning in development. */
throw new Error('It should never reach here');
Copy link
Member

Choose a reason for hiding this comment

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

We generally avoid blatant error in Vega-Lite.

Better fall through to use existing logic and optionally gracefully warn (with log.warn + add the message "invalid to messages.ts) if you think it's really worth warning.

I think it's ok to just fall through though.

if (isSignalRef(padding)) {
return {signal: `(1 - (${padding.signal})) * ${sizeChannel}`};
} else if (isNumber(padding)) {
return {signal: `(1 - (${padding})) * ${sizeChannel}`};
Copy link
Member

@kanitw kanitw May 10, 2023

Choose a reason for hiding this comment

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

- signal: `(1 - (${padding})) * ${sizeChannel}`
+ signal: `${1 - padding} * ${sizeChannel}`

Copy link
Member

@kanitw kanitw May 10, 2023

Choose a reason for hiding this comment

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

I think we don't need these parentheses and 1 - xxx math for this number case. We can just put in the number.

@yhoonkim yhoonkim enabled auto-merge (squash) May 10, 2023 19:27
@kanitw kanitw disabled auto-merge May 11, 2023 16:08
@kanitw kanitw enabled auto-merge (squash) May 11, 2023 16:10
@kanitw kanitw merged commit 309af9a into main May 11, 2023
@kanitw kanitw deleted the yh/fix-initial-1D-bar-size branch May 11, 2023 16:14
BradyJ27 pushed a commit to BradyJ27/vega-lite that referenced this pull request Oct 19, 2023
Co-authored-by: GitHub Actions Bot <vega-actions-bot@users.noreply.github.com>
Co-authored-by: Kanit Wongsuphasawat <kanitw@gmail.com>
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