From e11e1588dc3a26033112e2f3b8542c85f214ff24 Mon Sep 17 00:00:00 2001 From: Kanit Wongsuphasawat Date: Mon, 1 Feb 2021 22:00:24 -0800 Subject: [PATCH] fix: remove redundant code that leads to bugs + improve test coverage --- site/docs/mark/rect.md | 2 +- src/channeldef.ts | 16 +++++++++++----- src/compile/mark/encode/position-rect.ts | 15 +++------------ test/compile/mark/bar.test.ts | 18 ++++++++++++++++++ test/compile/mark/text.test.ts | 16 ++++++++++++++++ 5 files changed, 49 insertions(+), 18 deletions(-) diff --git a/site/docs/mark/rect.md b/site/docs/mark/rect.md index 8eb16320ff..6c5c09044e 100644 --- a/site/docs/mark/rect.md +++ b/site/docs/mark/rect.md @@ -47,7 +47,7 @@ The `rect` mark represents an arbitrary rectangle. A rect mark definition can contain any [standard mark properties](mark.html#mark-def) and the following special properties: -{% include table.html props="align,baseline,cornerRadius" source="MarkConfig" %} +{% include table.html props="width,height,align,baseline,cornerRadius" source="MarkConfig" %} ## Examples diff --git a/src/channeldef.ts b/src/channeldef.ts index 360b543381..1a34b9ae70 100644 --- a/src/channeldef.ts +++ b/src/channeldef.ts @@ -47,7 +47,7 @@ import { Y, Y2 } from './channel'; -import {getMarkConfig} from './compile/common'; +import {getMarkConfig, getMarkPropOrConfig} from './compile/common'; import {isCustomFormatType} from './compile/format'; import {CompositeAggregate} from './compositemark'; import {Config} from './config'; @@ -548,7 +548,8 @@ export function getBandSize({ fieldDef2, markDef: mark, config, - scaleType + scaleType, + useVlSizeChannel }: { channel: PositionScaleChannel | PolarPositionScaleChannel; fieldDef: ChannelDef; @@ -556,10 +557,15 @@ export function getBandSize({ markDef: MarkDef; config: Config; scaleType: ScaleType; + useVlSizeChannel?: boolean; }): number | RelativeBandSize | SignalRef { - const dimensionSize = mark[getSizeChannel(channel)]; - if (dimensionSize !== undefined) { - return dimensionSize; + const sizeChannel = getSizeChannel(channel); + const size = getMarkPropOrConfig(useVlSizeChannel ? 'size' : sizeChannel, mark, config, { + vgChannel: sizeChannel + }); + + if (size !== undefined) { + return size; } if (isFieldDef(fieldDef)) { diff --git a/src/compile/mark/encode/position-rect.ts b/src/compile/mark/encode/position-rect.ts index 0825abfe7a..95a2042156 100644 --- a/src/compile/mark/encode/position-rect.ts +++ b/src/compile/mark/encode/position-rect.ts @@ -137,22 +137,13 @@ function positionAndSize( // use "size" channel for bars, if there is orient and the channel matches the right orientation const useVlSizeChannel = (orient === 'horizontal' && channel === 'y') || (orient === 'vertical' && channel === 'x'); - const sizeFromMarkOrConfig = getMarkPropOrConfig(useVlSizeChannel ? 'size' : vgSizeChannel, markDef, config, { - vgChannel: vgSizeChannel - }); - // Use size encoding / mark property / config if it exists let sizeMixins; - if (encoding.size || sizeFromMarkOrConfig !== undefined) { + if (encoding.size || markDef.size) { if (useVlSizeChannel) { sizeMixins = nonPosition('size', model, { vgChannel: vgSizeChannel, - defaultRef: isRelativeBandSize(sizeFromMarkOrConfig) - ? { - scale: scaleName, - band: sizeFromMarkOrConfig.band - } - : signalOrValueRef(sizeFromMarkOrConfig) + defaultRef: signalOrValueRef(markDef.size) }); } else { log.warn(log.message.cannotApplySizeToNonOrientedMark(markDef.type)); @@ -160,7 +151,7 @@ function positionAndSize( } // Otherwise, apply default value - const bandSize = getBandSize({channel, fieldDef, markDef, config, scaleType: scale?.get('type')}); + const bandSize = getBandSize({channel, fieldDef, markDef, config, scaleType: scale?.get('type'), useVlSizeChannel}); sizeMixins = sizeMixins || {[vgSizeChannel]: defaultSizeRef(vgSizeChannel, scaleName, scale, config, bandSize)}; diff --git a/test/compile/mark/bar.test.ts b/test/compile/mark/bar.test.ts index 8eee158b59..c72a4a6f22 100644 --- a/test/compile/mark/bar.test.ts +++ b/test/compile/mark/bar.test.ts @@ -6,6 +6,24 @@ import {defaultBarConfig} from '../../../src/mark'; import {parseUnitModelWithScaleAndLayoutSize} from '../../util'; describe('Mark: Bar', () => { + describe('1D vertical', () => { + const model = parseUnitModelWithScaleAndLayoutSize({ + data: {url: 'data/cars.json'}, + mark: {type: 'bar', width: {band: 0.5}}, + encoding: { + y: {type: 'quantitative', field: 'Acceleration', aggregate: 'mean'} + } + }); + const props = bar.encodeEntry(model); + + it('should draw bar, with y from zero to field value and with band value for x/width', () => { + expect(props.width).toEqual({mult: 0.5, field: {group: 'width'}}); + expect(props.y).toEqual({scale: 'y', field: 'mean_Acceleration'}); + expect(props.y2).toEqual({scale: 'y', value: 0}); + expect(props.height).toBeUndefined(); + }); + }); + describe('simple vertical', () => { const model = parseUnitModelWithScaleAndLayoutSize({ data: {url: 'data/cars.json'}, diff --git a/test/compile/mark/text.test.ts b/test/compile/mark/text.test.ts index 3164f631ba..338b103183 100644 --- a/test/compile/mark/text.test.ts +++ b/test/compile/mark/text.test.ts @@ -25,6 +25,22 @@ describe('Mark: Text', () => { }); }); + it('should use stack_mid on theta for stacked theta', () => { + // This is a simplified example for stacked text. + // In reality this will be used as stacked's overlayed marker + const model = parseUnitModelWithScaleAndLayoutSize({ + mark: 'text', + encoding: { + theta: {field: 'value', type: 'quantitative', stack: true}, + color: {field: 'b', type: 'ordinal'} + }, + data: {url: 'data/barley.json'} + }); + + const props = text.encodeEntry(model); + expect(props.theta).toEqual({signal: 'scale("theta", 0.5 * datum["value_start"] + 0.5 * datum["value_end"])'}); + }); + describe('with stacked y', () => { // This is a simplified example for stacked text. // In reality this will be used as stacked's overlayed marker