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: fix thin bar problem (#8350) #8349

Merged
merged 5 commits into from
Aug 11, 2022
Merged

fix: fix thin bar problem (#8350) #8349

merged 5 commits into from
Aug 11, 2022

Conversation

kanitw
Copy link
Member

@kanitw kanitw commented Aug 9, 2022

See the added bar_group_thin example before and after the behavior fix commit.

(Other Vega diffs do not produce image diffs, so this shouldn't introduce any regression.)

📦 Published PR as canary version: 5.4.1--canary.8349.4227eff.0

✨ Test out this PR locally via:

npm install vega-lite@5.4.1--canary.8349.4227eff.0
# or 
yarn add vega-lite@5.4.1--canary.8349.4227eff.0

Version

Published prerelease version: v5.4.1-next.1

Changelog

🐛 Bug Fix

Authors: 5

@kanitw kanitw changed the title Fix thin bar problem #8350 Fix thin bar problem Aug 9, 2022
@kanitw kanitw marked this pull request as ready for review August 9, 2022 00:54
@kanitw kanitw requested review from yhoonkim and a team August 9, 2022 00:54
@kanitw kanitw changed the title #8350 Fix thin bar problem fix: fix thin bar problem (#8350) Aug 9, 2022
Copy link
Contributor

@yhoonkim yhoonkim left a comment

Choose a reason for hiding this comment

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

Made a minor comment due to my lack of the context. Feel free to merge if you are sure about it.

@@ -156,7 +162,8 @@ function positionAndSize(
If band is 0.6, the the x/y position in such case should be `(1 - band) / 2` = 0.2
*/

const defaultBandAlign = scale?.get('type') !== 'band' || !('band' in sizeMixins[vgSizeChannel]) ? 'middle' : 'top';
const defaultBandAlign =
scale?.get('type') === 'band' && isRelativeBandSize(bandSize) && !hasFixedSizeMixins ? 'top' : 'middle';
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain if
('band' in sizeMixins[vgSizeChannel]) is equivalent isRelativeBandSize(bandSize) && !hasFixedSizeMixins?

I wonder if this logic change is correct and necessary

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, since we output {signal: max(0.25, bandwidth('x'))} instead of {"scale": "x", "band": 1}, the old logic that checks for "band" doesn't work anymore.

I tracked how band insizeMixins[vgSizeChannel]` was generated and found that

band in sizeMixins[vgSizeChannel] only happens if the "bandSize" is relative and there is no size from mark/encoding (i'll rename the variable to hasSizeFromMarkOrEncoding).

@kanitw kanitw enabled auto-merge (squash) August 11, 2022 19:35
@kanitw kanitw merged commit 5fb894a into next Aug 11, 2022
@kanitw kanitw deleted the kw/thin-bar branch August 11, 2022 19:39
@vega-org-bot
Copy link
Collaborator

🚀 PR was released in v5.5.0 🚀

@vega-org-bot vega-org-bot added released This PR has been released by auto shipit and removed prerelease labels Aug 15, 2022
@kanitw kanitw restored the kw/thin-bar branch September 22, 2022 04:55
@kanitw kanitw deleted the kw/thin-bar branch September 26, 2023 23:41
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released This PR has been released by auto shipit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants