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

Jump to bottom

Do not ignore x2's title #3766

Merged
merged 3 commits into from
May 19, 2018
Merged

Do not ignore x2's title #3766

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5a2d7b8
Do not ignore x2's title
kanitw May 18, 2018
f9a8308
Refactor to use common mergeTitle codebase
kanitw May 18, 2018
1a05ea2
add comment
kanitw May 19, 2018
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
29 changes: 16 additions & 13 deletions src/compile/axis/parse.ts
View file
Open in desktop
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 @@ -157,18 +157,21 @@ function getFieldDefTitle(model: UnitModel, channel: 'x' | 'y') {
const fieldDef = model.fieldDef(channel);
const fieldDef2 = model.fieldDef(channel2);

const titles = [
...(fieldDef && fieldDef.title ? [fieldDef.title] : []),
...(fieldDef2 && fieldDef2.title ? [fieldDef2.title] : [])
];

if (titles.length > 0) {
return titles.join(', ');
} else if (fieldDef && fieldDef.title !== undefined) { // explicit falsy value
return fieldDef.title;
} else if (fieldDef2 && fieldDef2.title !== undefined) { // explicit falsy value
return fieldDef2.title;
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) {
Copy link
Member

Choose a reason for hiding this comment

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

What is this about? Can you add a comment?

return title1;
} else if (title2 !== undefined) {
return title2;
}

return undefined;
}

Expand Down
12 changes: 8 additions & 4 deletions src/compile/common.ts
View file
Open in desktop
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
View file
Open in desktop
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