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.

Sign up for GitHub

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
df79ce6
add example bar_grouped_thin
kanitw Aug 9, 2022
448d8b3
chore: update examples [CI]
Aug 9, 2022
4cd5f2c
fix(#8350): fix thin bar when there are too many categories
kanitw Aug 9, 2022
f5dfbbd
chore: update examples [CI]
Aug 9, 2022
4227eff
rename `hasSizeFromMarkOrEncoding`
kanitw Aug 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions src/compile/mark/encode/position-rect.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,12 @@ function defaultSizeRef(
if (scale) {
const scaleType = scale.get('type');
if (scaleType === 'band') {
return {scale: scaleName, band: bandSize.band};
let bandWidth = `bandwidth('${scaleName}')`;
if (bandSize.band !== 1) {
bandWidth = `${bandSize.band} * ${bandWidth}`;
}
// TODO(#8351): make 0.25 here configurable
return {signal: `max(0.25, ${bandWidth})`};
} else if (bandSize.band !== 1) {
log.warn(log.message.cannotUseRelativeBandSizeWithNonBandScale(scaleType));
bandSize = undefined;
Expand Down Expand Up @@ -139,6 +144,7 @@ function positionAndSize(
log.warn(log.message.cannotApplySizeToNonOrientedMark(markDef.type));
}
}
const hasFixedSizeMixins = !!sizeMixins;

// Otherwise, apply default value
const bandSize = getBandSize({channel, fieldDef, markDef, config, scaleType: scale?.get('type'), useVlSizeChannel});
Expand All @@ -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).


const vgChannel = vgAlignedPositionChannel(channel, markDef, config, defaultBandAlign);
const center = vgChannel === 'xc' || vgChannel === 'yc';
Expand Down
28 changes: 11 additions & 17 deletions test/compile/mark/bar.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ describe('Mark: Bar', () => {

it('should draw bar, with y from zero to field value and with band value for x/width', () => {
expect(props.x).toEqual({scale: 'x', field: 'Origin'});
expect(props.width).toEqual({scale: 'x', band: 1});
expect(props.width).toEqual({signal: `max(0.25, bandwidth('x'))`});
expect(props.y).toEqual({scale: 'y', field: 'mean_Acceleration'});
expect(props.y2).toEqual({scale: 'y', value: 0});
expect(props.height).toBeUndefined();
Expand All @@ -37,7 +37,7 @@ describe('Mark: Bar', () => {
});
const props = bar.encodeEntry(model);
expect(props.x).toEqual({scale: 'x', field: 'Origin', offset: {scale: 'xOffset', field: 'SubOrigin'}});
expect(props.width).toEqual({scale: 'xOffset', band: 1});
expect(props.width).toEqual({signal: `max(0.25, bandwidth('xOffset'))`});
expect(props.y).toEqual({scale: 'y', field: 'mean_Acceleration'});
expect(props.y2).toEqual({scale: 'y', value: 0});
expect(props.height).toBeUndefined();
Expand Down Expand Up @@ -75,7 +75,7 @@ describe('Mark: Bar', () => {
const props = bar.encodeEntry(model);

expect(props.x).toEqual({scale: 'x', field: 'Origin'});
expect(props.width).toEqual({scale: 'x', band: 1});
expect(props.width).toEqual({signal: `max(0.25, bandwidth('x'))`});
expect(props.y).toEqual({scale: 'y', field: 'mean_Acceleration'});
expect(props.y2).toEqual({scale: 'y', value: 0});
expect(props.height).toBeUndefined();
Expand Down Expand Up @@ -154,7 +154,7 @@ describe('Mark: Bar', () => {

it('should draw bar from zero to field value and with band value for x/width', () => {
expect(props.y).toEqual({scale: 'y', field: 'Origin'});
expect(props.height).toEqual({scale: 'y', band: 1});
expect(props.height).toEqual({signal: `max(0.25, bandwidth('y'))`});
expect(props.x).toEqual({scale: 'x', field: 'mean_Acceleration'});
expect(props.x2).toEqual({scale: 'x', value: 0});
expect(props.width).toBeUndefined();
Expand All @@ -174,7 +174,7 @@ describe('Mark: Bar', () => {

it('should draw bar from zero to field value and with band value for x/width', () => {
expect(props.y).toEqual({scale: 'y', field: 'Origin', band: 0.2});
expect(props.height).toEqual({scale: 'y', band: 0.6});
expect(props.height).toEqual({signal: `max(0.25, 0.6 * bandwidth('y'))`});
expect(props.x).toEqual({scale: 'x', field: 'mean_Acceleration'});
expect(props.x2).toEqual({scale: 'x', value: 0});
expect(props.width).toBeUndefined();
Expand Down Expand Up @@ -464,7 +464,7 @@ describe('Mark: Bar', () => {

it('should draw bar with y', () => {
expect(props.y).toEqual({scale: 'y', field: 'bin_maxbins_10_Horsepower_range'});
expect(props.height).toEqual({scale: 'y', band: 1});
expect(props.height).toEqual({signal: `max(0.25, bandwidth('y'))`});
});
});

Expand All @@ -481,7 +481,7 @@ describe('Mark: Bar', () => {

it('should draw bar with y', () => {
expect(props.x).toEqual({scale: 'x', field: 'bin_maxbins_10_Horsepower_range'});
expect(props.width).toEqual({scale: 'x', band: 1});
expect(props.width).toEqual({signal: `max(0.25, bandwidth('x'))`});
});
});

Expand Down Expand Up @@ -620,10 +620,7 @@ describe('Mark: Bar', () => {
scale: 'x',
field: 'Origin'
});
expect(props.width).toEqual({
scale: 'x',
band: 1
});
expect(props.width).toEqual({signal: `max(0.25, bandwidth('x'))`});
});
});

Expand All @@ -645,10 +642,7 @@ describe('Mark: Bar', () => {
scale: 'y',
field: 'Origin'
});
expect(props.height).toEqual({
scale: 'y',
band: 1
});
expect(props.height).toEqual({signal: `max(0.25, bandwidth('y'))`});
});
});

Expand Down Expand Up @@ -918,9 +912,9 @@ describe('Mark: Bar', () => {
const props = bar.encodeEntry(model);

expect(props.x).toEqual({scale: 'x', field: 'Origin'});
expect(props.width).toEqual({scale: 'x', band: 1});
expect(props.width).toEqual({signal: `max(0.25, bandwidth('x'))`});
expect(props.y).toEqual({scale: 'y', field: 'Cylinders'});
expect(props.height).toEqual({scale: 'y', band: 1});
expect(props.height).toEqual({signal: `max(0.25, bandwidth('y'))`});
});
});

Expand Down
10 changes: 5 additions & 5 deletions test/compile/mark/rect.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ describe('Mark: Rect', () => {

it('should draw bar, with y from zero to field value and x band', () => {
expect(props.x).toEqual({scale: 'x', field: 'Origin'});
expect(props.width).toEqual({scale: 'x', band: 1});
expect(props.width).toEqual({signal: `max(0.25, bandwidth('x'))`});
expect(props.y).toEqual({scale: 'y', field: 'mean_Acceleration'});
expect(props.y2).toEqual({scale: 'y', value: 0});
expect(props.height).toBeUndefined();
Expand Down Expand Up @@ -170,7 +170,7 @@ describe('Mark: Rect', () => {

it('should draw bar from zero to field value and y band', () => {
expect(props.y).toEqual({scale: 'y', field: 'Origin'});
expect(props.height).toEqual({scale: 'y', band: 1});
expect(props.height).toEqual({signal: `max(0.25, bandwidth('y'))`});
expect(props.x).toEqual({scale: 'x', field: 'mean_Acceleration'});
expect(props.x2).toEqual({scale: 'x', value: 0});
expect(props.width).toBeUndefined();
Expand All @@ -192,7 +192,7 @@ describe('Mark: Rect', () => {
});
const props = rect.encodeEntry(model);
expect(props.y).toEqual({scale: 'y', field: 'Origin'});
expect(props.height).toEqual({scale: 'y', band: 1});
expect(props.height).toEqual({signal: `max(0.25, bandwidth('y'))`});
expect(props.x).toEqual({scale: 'x', field: 'mean_Acceleration'});
expect(props.x2).toEqual({scale: 'x', value: 0});
expect(props.width).toBeUndefined();
Expand Down Expand Up @@ -275,9 +275,9 @@ describe('Mark: Rect', () => {

it('should draw rect with x and y bands', () => {
expect(props.x).toEqual({scale: 'x', field: 'Cylinders'});
expect(props.width).toEqual({scale: 'x', band: 1});
expect(props.width).toEqual({signal: `max(0.25, bandwidth('x'))`});
expect(props.y).toEqual({scale: 'y', field: 'Origin'});
expect(props.height).toEqual({scale: 'y', band: 1});
expect(props.height).toEqual({signal: `max(0.25, bandwidth('y'))`});
});
});

Expand Down