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

Do not ignore x2's title #3766

Merged
merged 3 commits into from
May 19, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
31 changes: 27 additions & 4 deletions src/compile/axis/parse.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import {POSITION_SCALE_CHANNELS, PositionScaleChannel, X, Y} from '../../channel
import {FieldDefBase, toFieldDefBase} from '../../fielddef';
import {keys} from '../../util';
import {AxisOrient, VgAxis, VgAxisEncode} from '../../vega.schema';
import {getSpecifiedOrDefaultValue, mergeTitleFieldDefs, numberFormat, titleMerger} from '../common';
import {getSpecifiedOrDefaultValue, mergeTitle, mergeTitleComponent, mergeTitleFieldDefs, numberFormat} from '../common';
import {LayerModel} from '../layer';
import {parseGuideResolve} from '../resolve';
import {defaultTieBreaker, Explicit, mergeValuesWithExplicit} from '../split';
Expand Down Expand Up @@ -137,7 +137,7 @@ function mergeAxisComponent(merged: AxisComponent, child: AxisComponent): AxisCo
(v1: Explicit<any>, v2: Explicit<any>) => {
switch (prop) {
case 'title':
return titleMerger(v1, v2);
return mergeTitleComponent(v1, v2);
case 'gridScale':
return {
explicit: v1.explicit, // keep the old explicit
Expand All @@ -152,6 +152,28 @@ function mergeAxisComponent(merged: AxisComponent, child: AxisComponent): AxisCo
return merged;
}

function getFieldDefTitle(model: UnitModel, channel: 'x' | 'y') {
const channel2 = channel === 'x' ? 'x2' : 'y2';
const fieldDef = model.fieldDef(channel);
const fieldDef2 = model.fieldDef(channel2);

const title1 = fieldDef ? fieldDef.title : undefined;
const title2 = fieldDef2 ? fieldDef2.title : undefined;

if (title1 && title2) {
return mergeTitle(title1, title2);
} else if (title1) {
return title1;
} else if (title2) {
return title2;
} else if (title1 !== undefined) { // falsy value to disable config
return title1;
} else if (title2 !== undefined) { // falsy value to disable config
return title2;
}

return undefined;
}

function parseAxis(channel: PositionScaleChannel, model: UnitModel): AxisComponent {
const axis = model.axis(channel);
Expand All @@ -168,7 +190,7 @@ function parseAxis(channel: PositionScaleChannel, model: UnitModel): AxisCompone
// both VL axis.encoding and axis.labelAngle affect VG axis.encode
property === 'encode' ? !!axis.encoding || !!axis.labelAngle :
// title can be explicit if fieldDef.title is set
property === 'title' && value === model.fieldDef(channel).title ? true :
property === 'title' && value === getFieldDefTitle(model, channel) ? true :
// Otherwise, things are explicit if the returned value matches the specified property
value === axis[property];

Expand Down Expand Up @@ -242,7 +264,8 @@ function getProperty<K extends keyof AxisComponentProps>(property: K, specifiedA
const fieldDef2 = model.fieldDef(channel2);
// Keep undefined so we use default if title is unspecified.
// For other falsy value, keep them so we will hide the title.
const specifiedTitle = fieldDef.title !== undefined ? fieldDef.title :
const fieldDefTitle = getFieldDefTitle(model, channel);
const specifiedTitle = fieldDefTitle !== undefined ? fieldDefTitle :
specifiedAxis.title === undefined ? undefined : specifiedAxis.title;

return getSpecifiedOrDefaultValue<string | FieldDefBase<string>[]>(
Expand Down
12 changes: 8 additions & 4 deletions src/compile/common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,13 @@ export function mergeTitleFieldDefs(f1: FieldDefBase<string>[], f2: FieldDefBase
return merged;
}

export function titleMerger(
export function mergeTitle(title1: string, title2: string) {
return title1 === title2 ?
title1 : // if title is the same just use one of them
title1 + ', ' + title2; // join title with comma if different
}

export function mergeTitleComponent(
v1: Explicit<AxisTitleComponent>, v2: Explicit<AxisTitleComponent>
) {
if (isArray(v1.value) && isArray(v2.value)) {
Expand All @@ -192,9 +198,7 @@ export function titleMerger(
} else if (!isArray(v1.value) && !isArray(v2.value)) {
return {
explicit: v1.explicit, // keep the old explicit
value: v1.value === v2.value ?
v1.value : // if title is the same just use one of them
v1.value + ', ' + v2.value // join title with comma if different
value: mergeTitle(v1.value, v2.value)
};
}
/* istanbul ignore next: Condition should not happen -- only for warning in development. */
Expand Down
4 changes: 2 additions & 2 deletions src/compile/legend/parse.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import {Legend, LEGEND_PROPERTIES, VG_LEGEND_PROPERTIES} from '../../legend';
import {GEOJSON} from '../../type';
import {deleteNestedProperty, keys} from '../../util';
import {VgLegend, VgLegendEncode} from '../../vega.schema';
import {getSpecifiedOrDefaultValue, numberFormat, titleMerger} from '../common';
import {getSpecifiedOrDefaultValue, mergeTitleComponent, numberFormat} from '../common';
import {isUnitModel, Model} from '../model';
import {parseGuideResolve} from '../resolve';
import {Explicit, makeImplicit} from '../split';
Expand Down Expand Up @@ -184,7 +184,7 @@ export function mergeLegendComponent(mergedLegend: LegendComponent, childLegend:
(v1: Explicit<any>, v2: Explicit<any>): any => {
switch (prop) {
case 'title':
return titleMerger(v1, v2);
return mergeTitleComponent(v1, v2);
case 'type':
// There are only two types. If we have different types, then prefer symbol over gradient.
typeMerged = true;
Expand Down
59 changes: 59 additions & 0 deletions test/compile/axis/parse.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,24 @@ describe('Axis', function() {
}
});

it('should store the fieldDef title value if title = null, "", or false', function () {
Copy link
Member

Choose a reason for hiding this comment

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

why not use an arrow function here

Copy link
Member Author

Choose a reason for hiding this comment

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

You gonna call another clean all service later anyway, so I'll ignore this :p

Copy link
Member

Choose a reason for hiding this comment

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

Okay

for (const val of [null, '', false]) {
const model = parseUnitModelWithScale({
mark: "point",
encoding: {
x: {
field: "a",
type: "quantitative",
title: val as any // Need to cast as false is not valid, but we want to fall back gracefully
}
}
});
const axisComponent = parseUnitAxis(model);
assert.equal(axisComponent['x'].length, 1);
assert.equal(axisComponent['x'][0].explicit.title, val as any);
}
});

it('should store fieldDef.title as explicit', function () {
const model = parseUnitModelWithScale({
mark: "point",
Expand All @@ -146,6 +164,47 @@ describe('Axis', function() {
assert.equal(axisComponent['x'][0].explicit.title, 'foo');
});

it('should merge title of fieldDef and fieldDef2', function () {
const model = parseUnitModelWithScale({
mark: "bar",
encoding: {
x: {
Copy link
Member

Choose a reason for hiding this comment

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

Let's add a test where only x and only x2 have a title defined.

Copy link
Member Author

Choose a reason for hiding this comment

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

That doesn't work for other reasons, and I'm not gonna fix it in this PR. Feel free to file another issue.

Copy link
Member

Choose a reason for hiding this comment

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

Okay

field: "a",
type: "quantitative",
title: 'foo'
},
x2: {
field: "b",
type: "quantitative",
title: 'bar'
}
}
});
const axisComponent = parseUnitAxis(model);
assert.equal(axisComponent['x'].length, 1);
assert.equal(axisComponent['x'][0].explicit.title, 'foo, bar');
});

it('should use title of fieldDef2', function () {
const model = parseUnitModelWithScale({
mark: "bar",
encoding: {
x: {
field: "a",
type: "quantitative"
},
x2: {
field: "b",
type: "quantitative",
title: 'bar'
}
}
});
const axisComponent = parseUnitAxis(model);
assert.equal(axisComponent['x'].length, 1);
assert.equal(axisComponent['x'][0].explicit.title, 'bar');
});

it('should store both x and x2 for ranged mark', function () {
const model = parseUnitModelWithScale({
mark: "rule",
Expand Down