Skip to content

Commit

Permalink
fix: support x/yOffset without x/y
Browse files Browse the repository at this point in the history
  • Loading branch information
kanitw committed Oct 12, 2023
1 parent f42c534 commit 8bbb98c
Show file tree
Hide file tree
Showing 6 changed files with 134 additions and 38 deletions.
13 changes: 11 additions & 2 deletions src/compile/mark/encode/position-rect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,14 @@ function positionAndSize(
const hasSizeFromMarkOrEncoding = !!sizeMixins;

// Otherwise, apply default value
const bandSize = getBandSize({channel, fieldDef, markDef, config, scaleType: scale?.get('type'), useVlSizeChannel});
const bandSize = getBandSize({
channel,
fieldDef,
markDef,
config,
scaleType: (scale || offsetScale)?.get('type'),
useVlSizeChannel
});

sizeMixins = sizeMixins || {
[vgSizeChannel]: defaultSizeRef(
Expand All @@ -190,7 +197,9 @@ function positionAndSize(
*/

const defaultBandAlign =
scale?.get('type') === 'band' && isRelativeBandSize(bandSize) && !hasSizeFromMarkOrEncoding ? 'top' : 'middle';
(scale || offsetScale)?.get('type') === 'band' && isRelativeBandSize(bandSize) && !hasSizeFromMarkOrEncoding
? 'top'
: 'middle';

const vgChannel = vgAlignedPositionChannel(channel, markDef, config, defaultBandAlign);
const center = vgChannel === 'xc' || vgChannel === 'yc';
Expand Down
14 changes: 1 addition & 13 deletions src/compile/scale/parse.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import {getMainChannelFromOffsetChannel, isXorYOffset, ScaleChannel, SCALE_CHANNELS, SHAPE} from '../../channel';
import {ScaleChannel, SCALE_CHANNELS, SHAPE} from '../../channel';
import {getFieldOrDatumDef, ScaleDatumDef, TypedFieldDef} from '../../channeldef';
import {channelHasNestedOffsetScale} from '../../encoding';
import * as log from '../../log';
import {GEOSHAPE} from '../../mark';
import {
NON_TYPE_DOMAIN_RANGE_VEGA_SCALE_PROPERTIES,
Expand Down Expand Up @@ -56,17 +55,6 @@ function parseUnitScaleCore(model: UnitModel): ScaleComponentIndex {
}

let specifiedScale = fieldOrDatumDef && fieldOrDatumDef['scale'];
if (isXorYOffset(channel)) {
const mainChannel = getMainChannelFromOffsetChannel(channel);
if (!channelHasNestedOffsetScale(encoding, mainChannel)) {
// Don't generate scale when the offset encoding shouldn't yield a nested scale
if (specifiedScale) {
log.warn(log.message.offsetEncodingScaleIgnored(channel));
}
continue;
}
}

if (fieldOrDatumDef && specifiedScale !== null && specifiedScale !== false) {
specifiedScale ??= {};
const hasNestedOffsetScale = channelHasNestedOffsetScale(encoding, channel);
Expand Down
50 changes: 36 additions & 14 deletions src/compile/scale/range.ts
Original file line number Diff line number Diff line change
Expand Up @@ -221,11 +221,39 @@ function parseScheme(scheme: Scheme | SignalRef): RangeScheme {
return {scheme};
}

function fullWidthOrHeightRange(
channel: 'x' | 'y',
model: UnitModel,
scaleType: ScaleType,
{center}: {center?: boolean} = {}
) {
// If step is null, use zero to width or height.
// Note that we use SignalRefWrapper to account for potential merges and renames.
const sizeType = getSizeChannel(channel);
const sizeSignal = model.getName(sizeType);
const getSignalName = model.getSignalName.bind(model);

if (channel === Y && hasContinuousDomain(scaleType)) {
// For y continuous scale, we have to start from the height as the bottom part has the max value.
return center
? [
SignalRefWrapper.fromName(name => `${getSignalName(name)}/2`, sizeSignal),
SignalRefWrapper.fromName(name => `-${getSignalName(name)}/2`, sizeSignal)

Check warning on line 241 in src/compile/scale/range.ts

View check run for this annotation

Codecov / codecov/patch

src/compile/scale/range.ts#L239-L241

Added lines #L239 - L241 were not covered by tests
]
: [SignalRefWrapper.fromName(getSignalName, sizeSignal), 0];
} else {
return center
? [
SignalRefWrapper.fromName(name => `-${getSignalName(name)}/2`, sizeSignal),
SignalRefWrapper.fromName(name => `${getSignalName(name)}/2`, sizeSignal)
]
: [0, SignalRefWrapper.fromName(getSignalName, sizeSignal)];
}
}

function defaultRange(channel: ScaleChannel, model: UnitModel): VgRange {
const {size, config, mark, encoding} = model;

const getSignalName = model.getSignalName.bind(model);

const {type} = getFieldOrDatumDef(encoding[channel]) as ScaleFieldDef<string> | ScaleDatumDef;

const mergedScaleCmpt = model.getScaleComponent(channel);
Expand All @@ -245,18 +273,7 @@ function defaultRange(channel: ScaleChannel, model: UnitModel): VgRange {
}
}

// If step is null, use zero to width or height.
// Note that we use SignalRefWrapper to account for potential merges and renames.

const sizeType = getSizeChannel(channel);
const sizeSignal = model.getName(sizeType);

if (channel === Y && hasContinuousDomain(scaleType)) {
// For y continuous scale, we have to start from the height as the bottom part has the max value.
return [SignalRefWrapper.fromName(getSignalName, sizeSignal), 0];
} else {
return [0, SignalRefWrapper.fromName(getSignalName, sizeSignal)];
}
return fullWidthOrHeightRange(channel, model, scaleType);
}

case XOFFSET:
Expand Down Expand Up @@ -374,6 +391,11 @@ function getOffsetStep(step: Step, offsetScaleType: ScaleType) {
function getOffsetRange(channel: string, model: UnitModel, offsetScaleType: ScaleType): VgRange {
const positionChannel = channel === XOFFSET ? 'x' : 'y';
const positionScaleCmpt = model.getScaleComponent(positionChannel);

if (!positionScaleCmpt) {
return fullWidthOrHeightRange(positionChannel, model, offsetScaleType, {center: true});
}

const positionScaleType = positionScaleCmpt.get('type');
const positionScaleName = model.scaleName(positionChannel);

Expand Down
16 changes: 8 additions & 8 deletions src/stack.ts
Original file line number Diff line number Diff line change
Expand Up @@ -176,16 +176,16 @@ export function stack(m: Mark | MarkDef, encoding: Encoding<string>): StackPrope
groupbyChannels.push(dimensionChannel);
groupbyFields.add(dimensionField);
}
}

const dimensionOffsetChannel = dimensionChannel === 'x' ? 'xOffset' : 'yOffset';
const dimensionOffsetDef = encoding[dimensionOffsetChannel];
const dimensionOffsetField = isFieldDef(dimensionOffsetDef) ? vgField(dimensionOffsetDef, {}) : undefined;
const dimensionOffsetChannel = dimensionChannel === 'x' ? 'xOffset' : 'yOffset';
const dimensionOffsetDef = encoding[dimensionOffsetChannel];
const dimensionOffsetField = isFieldDef(dimensionOffsetDef) ? vgField(dimensionOffsetDef, {}) : undefined;

if (dimensionOffsetField && dimensionOffsetField !== stackedField) {
// avoid grouping by the stacked field
groupbyChannels.push(dimensionOffsetChannel);
groupbyFields.add(dimensionOffsetField);
}
if (dimensionOffsetField && dimensionOffsetField !== stackedField) {
// avoid grouping by the stacked field
groupbyChannels.push(dimensionOffsetChannel);
groupbyFields.add(dimensionOffsetField);
}

// If the dimension has offset, don't stack anymore
Expand Down
42 changes: 42 additions & 0 deletions test/compile/scale/range.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,48 @@ describe('compile/scale', () => {
);
});

it('should return [-width/2, width/2] for xOffset without x', () => {
const model = parseUnitModelWithScaleExceptRange({
mark: 'bar',
encoding: {
xOffset: {field: 'xSub', type: 'nominal'},
y: {field: 'y', type: 'nominal'}
}
});

expect(parseRangeForChannel('xOffset', model)).toEqual(
makeImplicit([
{
signal: '-width/2'
},
{
signal: 'width/2'
}
])
);
});

it('should return [-height/2, height/2] for yOffset without y', () => {
const model = parseUnitModelWithScaleExceptRange({
mark: 'bar',
encoding: {
yOffset: {field: 'xSub', type: 'nominal'},
x: {field: 'y', type: 'nominal'}
}
});

expect(parseRangeForChannel('yOffset', model)).toEqual(
makeImplicit([
{
signal: '-height/2'
},
{
signal: 'height/2'
}
])
);
});

it('should return padded duration range when there is a nested offset with year time scale and default padding', () => {
const model = parseUnitModelWithScaleExceptRange({
mark: 'bar',
Expand Down
37 changes: 36 additions & 1 deletion test/stack.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {NonArgAggregateOp} from '../src/aggregate';
import {DETAIL, X, Y} from '../src/channel';
import {DETAIL, X, Y, YOFFSET} from '../src/channel';
import * as log from '../src/log';
import {ARC, AREA, BAR, PRIMITIVE_MARKS, RECT} from '../src/mark';
import {ScaleType} from '../src/scale';
Expand Down Expand Up @@ -327,6 +327,41 @@ describe('stack', () => {
}
});

it('should be correct for grouped bar', () => {
for (const stackableMark of [BAR, AREA]) {
const spec: TopLevel<NormalizedUnitSpec> = {
data: {url: 'data/barley.json'},
mark: stackableMark,
encoding: {
x: {field: 'yield', type: 'quantitative'},
y: {field: 'variety', type: 'nominal'},
yOffset: {field: 'site', type: 'nominal'},
color: {field: 'site', type: 'nominal'}
}
};
const _stack = stack(spec.mark, spec.encoding);
expect(_stack.fieldChannel).toBe(X);
expect(_stack.groupbyChannels).toEqual([Y, YOFFSET]);
}
});

it('should be correct for grouped bar without nesting', () => {
for (const stackableMark of [BAR, AREA]) {
const spec: TopLevel<NormalizedUnitSpec> = {
data: {url: 'data/barley.json'},
mark: stackableMark,
encoding: {
x: {field: 'yield', type: 'quantitative'},
yOffset: {field: 'site', type: 'nominal'},
color: {field: 'site', type: 'nominal'}
}
};
const _stack = stack(spec.mark, spec.encoding);
expect(_stack.fieldChannel).toBe(X);
expect(_stack.groupbyChannels).toEqual([YOFFSET]);
}
});

it('should stack even for two plain quantitatives with the orient on the axes', () => {
for (const mark of [BAR, AREA]) {
const spec: TopLevel<NormalizedUnitSpec> = {
Expand Down

0 comments on commit 8bbb98c

Please sign in to comment.