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

Add vertical-align in format expression #900

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
+182 −29
Open

Add vertical-align in format expression #900

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
d785d2b
Add vertical-align to v8.json
stanislawpuda-tomtom Oct 2, 2024
cfd9111
Add vertical-align to format definition
stanislawpuda-tomtom Oct 2, 2024
7593fa9
Add static value validation
stanislawpuda-tomtom Oct 2, 2024
6065c88
Add support for image
stanislawpuda-tomtom Oct 7, 2024
a4630d0
update vertical-align description
stanislawpuda-tomtom Nov 12, 2024
071c277
Merge branch 'main' of https://github.com/tomtom-forks/maplibre-style…
stanislawpuda-tomtom Nov 12, 2024
1a5cf7c
add changelog
stanislawpuda-tomtom Nov 12, 2024
4460a7e
Merge branch 'main' of https://github.com/tomtom-forks/maplibre-style…
stanislawpuda-tomtom Nov 27, 2024
695efdf
add image section `vertical-align` test
stanislawpuda-tomtom Nov 28, 2024
c74afe5
change `baseline` to `bottom`
stanislawpuda-tomtom Nov 28, 2024
b55daaa
add `vertical-align` to sdk support table
stanislawpuda-tomtom Nov 28, 2024
2429fe8
add links to maplibre native issue
stanislawpuda-tomtom Nov 28, 2024
4cf94a5
review fixes
stanislawpuda-tomtom Nov 28, 2024
8e957bd
Merge branch 'main' into issue-832-format-expression-vertical-align
stanislawpuda-tomtom Nov 28, 2024
37e1178
Merge branch 'main' into issue-832-format-expression-vertical-align
stanislawpuda-tomtom Dec 9, 2024
ef5ac81
Define `vertical-align` as set of possible values instead of `string`
stanislawpuda-tomtom Dec 16, 2024
653c90a
Merge branch 'main' into issue-832-format-expression-vertical-align
stanislawpuda-tomtom Dec 16, 2024
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
## main

### ✨ Features and improvements
- Add `vertical-align` option to `format` expression ([#832](https://github.com/maplibre/maplibre-style-spec/issues/832))
- _...Add new stuff here..._

### 🐞 Bug fixes
Expand Down
31 changes: 27 additions & 4 deletions src/expression/definitions/format.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import {
ColorType,
ResolvedImageType,
} from '../types';
import {Formatted, FormattedSection} from '../types/formatted';
import {Formatted, FormattedSection, VERTICAL_ALIGN_OPTIONS, VerticalAlign} from '../types/formatted';
import {valueToString, typeOf} from '../values';

import type {Expression} from '../expression';
Expand All @@ -22,6 +22,7 @@ type FormattedSectionExpression = {
scale: Expression | null;
font: Expression | null;
textColor: Expression | null;
verticalAlign: Expression | null;
};

export class FormatExpression implements Expression {
Expand Down Expand Up @@ -69,10 +70,21 @@ export class FormatExpression implements Expression {
if (!textColor) return null;
}

let verticalAlign = null;
if (arg['vertical-align']) {
if (typeof arg['vertical-align'] === 'string' && !VERTICAL_ALIGN_OPTIONS.includes(arg['vertical-align'] as VerticalAlign)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this if be moved to the parse code to keep this as clean as the other if statements?

Copy link
Author

@stanislawpuda-tomtom stanislawpuda-tomtom Dec 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I did try to move it to parse - however then it shouldn't be placed in parse fn itself, but I found that it is better to add enum validation, like for array in other places.

However, to make it possible there are multiple changes that has to be made. Moreover the validation for enum is a bit different, because other validations are based on types, and enum validation requires access to the value, so it is possible to compare with enum values.

You can see how it looks here: tomtom-forks#2

After making this try I actually think that having it in format.ts, as it is now, is better and much simpler. Also in similar fashion the validation is implemented for interpolate expression (link to the file).

return context.error(`'vertical-align' must be one of: 'bottom', 'center', 'top' but found '${arg['vertical-align']}' instead.`) as null;
}

verticalAlign = context.parse(arg['vertical-align'], 1, StringType);
if (!verticalAlign) return null;
}

const lastExpression = sections[sections.length - 1];
lastExpression.scale = scale;
lastExpression.font = font;
lastExpression.textColor = textColor;
lastExpression.verticalAlign = verticalAlign;
} else {
const content = context.parse(args[i], 1, ValueType);
if (!content) return null;
Expand All @@ -82,7 +94,7 @@ export class FormatExpression implements Expression {
return context.error('Formatted text type must be \'string\', \'value\', \'image\' or \'null\'.') as null;

nextTokenMayBeObject = true;
sections.push({content, scale: null, font: null, textColor: null});
sections.push({content, scale: null, font: null, textColor: null, verticalAlign: null});
}
}

Expand All @@ -93,15 +105,23 @@ export class FormatExpression implements Expression {
const evaluateSection = section => {
const evaluatedContent = section.content.evaluate(ctx);
if (typeOf(evaluatedContent) === ResolvedImageType) {
return new FormattedSection('', evaluatedContent, null, null, null);
return new FormattedSection(
'',
evaluatedContent,
null,
null,
null,
section.verticalAlign ? section.verticalAlign.evaluate(ctx) : null
Copy link
Collaborator

Choose a reason for hiding this comment

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

How is this different than other properties? Why not passing null here?

Copy link
Author

Choose a reason for hiding this comment

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

This is the instantiation of FormattedSection for image. All other properties are null, because they only applies to text (font etc.), but vertical-align can be applied to both image and text section.

);
}

return new FormattedSection(
valueToString(evaluatedContent),
null,
section.scale ? section.scale.evaluate(ctx) : null,
section.font ? section.font.evaluate(ctx).join(',') : null,
section.textColor ? section.textColor.evaluate(ctx) : null
section.textColor ? section.textColor.evaluate(ctx) : null,
section.verticalAlign ? section.verticalAlign.evaluate(ctx) : null
);
};

Expand All @@ -120,6 +140,9 @@ export class FormatExpression implements Expression {
if (section.textColor) {
fn(section.textColor);
}
if (section.verticalAlign) {
fn(section.verticalAlign);
}
}
}

Expand Down
9 changes: 7 additions & 2 deletions src/expression/types/formatted.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,24 @@
import type {Color} from '../../expression/types/color';
import type {ResolvedImage} from '../types/resolved_image';

export const VERTICAL_ALIGN_OPTIONS = ['bottom', 'center', 'top'] as const;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this be defined in the v8 spec file and not here?

Copy link
Author

Choose a reason for hiding this comment

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

I don't see a way how to do it - there is no other expression where this kind of definition lives in v8.json (or I can't find it). For interpolate expression type definition lives in the code (

maplibre-style-spec/src/expression/definitions/interpolate.ts

Line 17 in 93a8cd3

export type InterpolationType = {
).

export type VerticalAlign = typeof VERTICAL_ALIGN_OPTIONS[number];

export class FormattedSection {
text: string;
image: ResolvedImage | null;
scale: number | null;
fontStack: string | null;
textColor: Color | null;
verticalAlign: VerticalAlign | null;

constructor(text: string, image: ResolvedImage | null, scale: number | null, fontStack: string | null, textColor: Color | null) {
constructor(text: string, image: ResolvedImage | null, scale: number | null, fontStack: string | null, textColor: Color | null, verticalAlign: VerticalAlign | null) {
this.text = text;
this.image = image;
this.scale = scale;
this.fontStack = fontStack;
this.textColor = textColor;
this.verticalAlign = verticalAlign;
}
}

Expand All @@ -25,7 +30,7 @@ export class Formatted {
}

static fromString(unformatted: string): Formatted {
return new Formatted([new FormattedSection(unformatted, null, null, null, null)]);
return new Formatted([new FormattedSection(unformatted, null, null, null, null, null)]);
}

isEmpty(): boolean {
Expand Down
9 changes: 7 additions & 2 deletions src/reference/v8.json
View file
Open in desktop
HarelM marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -3226,10 +3226,10 @@
}
},
"format": {
"doc": "Returns a `formatted` string for displaying mixed-format text in the `text-field` property. The input may contain a string literal or expression, including an [`'image'`](#image) expression. Strings may be followed by a style override object that supports the following properties:\n\n- `\"text-font\"`: Overrides the font stack specified by the root layout property.\n\n- `\"text-color\"`: Overrides the color specified by the root paint property.\n\n- `\"font-scale\"`: Applies a scaling factor on `text-size` as specified by the root layout property.\n\n - [Change the case of labels](https://maplibre.org/maplibre-gl-js/docs/examples/change-case-of-labels/)\n\n - [Display and style rich text labels](https://maplibre.org/maplibre-gl-js/docs/examples/display-and-style-rich-text-labels/)",
"doc": "Returns a `formatted` string for displaying mixed-format text in the `text-field` property. The input may contain a string literal or expression, including an [`'image'`](#image) expression. Strings may be followed by a style override object that supports the following properties:\n\n- `\"text-font\"`: Overrides the font stack specified by the root layout property.\n\n- `\"text-color\"`: Overrides the color specified by the root paint property.\n\n- `\"font-scale\"`: Applies a scaling factor on `text-size` as specified by the root layout property.\n\n- `\"vertical-align\"`: Aligns vertically text section or image in relation to the row it belongs to. Possible values are: \n\t- `\"bottom\"` *default*: text baseline or image bottom are in line.\n\t- `\"center\"`: image center or text center are in line.\n\t- `\"top\"`: image top and text top are in line.\n\n - [Change the case of labels](https://maplibre.org/maplibre-gl-js/docs/examples/change-case-of-labels/)\n\n - [Display and style rich text labels](https://maplibre.org/maplibre-gl-js/docs/examples/display-and-style-rich-text-labels/)",
"example": {
"syntax": {
"method": ["value", "{ \"text-font\": string, \"text-color\": color, \"font-scale\": number }", "..."],
"method": ["value", "{ \"text-font\": string, \"text-color\": color, \"font-scale\": number, \"vertical-align\": string }", "..."],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we put enum possible values instead of string? Not sure how the handled in different places...

Copy link
Author

Choose a reason for hiding this comment

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

I changed string to list of possible values. In other places I found both approaches:

"result": "formatted"
},
"value": ["format", ["upcase", ["get", "FacilityName"]], {"font-scale": 0.8}, "\n\n", {}, ["downcase", ["get", "Comments"]], {"font-scale": 0.6}]
Expand All @@ -3256,6 +3256,11 @@
"android": "7.3.0",
"ios": "4.10.0"
},
"vertical-align": {
"js": "https://github.com/maplibre/maplibre-gl-js/issues/5043",
"android": "https://github.com/maplibre/maplibre-native/issues/3055",
"ios": "https://github.com/maplibre/maplibre-native/issues/3055"
},
"image": {
"js": "1.6.0",
"android": "8.6.0",
Expand Down
24 changes: 20 additions & 4 deletions test/integration/expression/tests/format/basic/test.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@
"d",
{
"text-color": "rgb(0, 255, 0)"
},
"e",
{
"vertical-align": "center"
}
],
"inputs": [
Expand All @@ -43,21 +47,24 @@
"image": null,
"scale": null,
"fontStack": null,
"textColor": null
"textColor": null,
"verticalAlign": null
},
{
"text": "b",
"image": null,
"scale": 2,
"fontStack": null,
"textColor": null
"textColor": null,
"verticalAlign": null
},
{
"text": "c",
"image": null,
"scale": null,
"fontStack": "a,b",
"textColor": null
"textColor": null,
"verticalAlign": null
},
{
"text": "d",
Expand All @@ -69,7 +76,16 @@
"g": 1,
"b": 0,
"a": 1
}
},
"verticalAlign": null
},
{
"text": "e",
"image": null,
"scale": null,
"fontStack": null,
"textColor": null,
"verticalAlign": "center"
}
]
}
Expand Down
9 changes: 6 additions & 3 deletions test/integration/expression/tests/format/coercion/test.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,21 +44,24 @@
"image": null,
"scale": null,
"fontStack": null,
"textColor": null
"textColor": null,
"verticalAlign": null
},
{
"text": "1",
"image": null,
"scale": null,
"fontStack": null,
"textColor": null
"textColor": null,
"verticalAlign": null
},
{
"text": "true",
"image": null,
"scale": null,
"fontStack": null,
"textColor": null
"textColor": null,
"verticalAlign": null
}
]
}
Expand Down
6 changes: 4 additions & 2 deletions test/integration/expression/tests/format/data-driven-font/test.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@
"image": null,
"scale": 1.5,
"fontStack": null,
"textColor": null
"textColor": null,
"verticalAlign": null
}
]
},
Expand All @@ -53,7 +54,8 @@
"image": null,
"scale": 0.5,
"fontStack": null,
"textColor": null
"textColor": null,
"verticalAlign": null
}
]
}
Expand Down
64 changes: 64 additions & 0 deletions test/integration/expression/tests/format/data-driven-vertical-align/test.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
{
"expression": [
"format",
"a",
{
"vertical-align": [
"get",
"vertical-align"
]
}
],
"inputs": [
[
{},
{
"properties": {
"vertical-align": "center"
}
}
],
[
{},
{
"properties": {
"vertical-align": "top"
}
}
]
],
"expected": {
"compiled": {
"result": "success",
"isFeatureConstant": false,
"isZoomConstant": true,
"type": "formatted"
},
"outputs": [
{
"sections": [
{
"text": "a",
"image": null,
"scale": null,
"fontStack": null,
"textColor": null,
"verticalAlign": "center"
}
]
},
{
"sections": [
{
"text": "a",
"image": null,
"scale": null,
"fontStack": null,
"textColor": null,
"verticalAlign": "top"
}
]
}
]
}
}
11 changes: 8 additions & 3 deletions test/integration/expression/tests/format/image-sections/test.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,10 @@
[
"image",
"beach-11"
]
],
{
"vertical-align": "center"
}
],
"inputs": [
[
Expand Down Expand Up @@ -38,7 +41,8 @@
},
"scale": null,
"fontStack": null,
"textColor": null
"textColor": null,
"verticalAlign": null
},
{
"text": "",
Expand All @@ -48,7 +52,8 @@
},
"scale": null,
"fontStack": null,
"textColor": null
"textColor": null,
"verticalAlign": "center"
}
]
}
Expand Down
9 changes: 6 additions & 3 deletions test/integration/expression/tests/format/implicit-coerce/test.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,8 @@
"image": null,
"scale": null,
"fontStack": null,
"textColor": null
"textColor": null,
"verticalAlign": null
}
]
},
Expand All @@ -67,7 +68,8 @@
"image": null,
"scale": null,
"fontStack": null,
"textColor": null
"textColor": null,
"verticalAlign": null
}
]
},
Expand All @@ -78,7 +80,8 @@
"image": null,
"scale": null,
"fontStack": null,
"textColor": null
"textColor": null,
"verticalAlign": null
}
]
}
Expand Down
Loading
Loading