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

Add vertical-align in format expression #900

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
20 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
7adf081
simplify vertical-align options description
stanislawpuda-tomtom Dec 23, 2024
56c2eed
update example
stanislawpuda-tomtom Dec 23, 2024
f2595e9
link design proposal for more details
stanislawpuda-tomtom Dec 23, 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
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
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)) {
HarelM marked this conversation as resolved.
Show resolved Hide resolved
return context.error(`\`vertical-align\` must be one of: 'bottom', 'center', 'top' but found '${arg['vertical-align']}' instead.`) as null;
HarelM marked this conversation as resolved.
Show resolved Hide resolved
}

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
HarelM marked this conversation as resolved.
Show resolved Hide resolved
);
}

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
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;
HarelM marked this conversation as resolved.
Show resolved Hide resolved
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
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\"`: Align 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/)",
HarelM marked this conversation as resolved.
Show resolved Hide resolved
"example": {
Copy link
Collaborator

@louwers louwers Dec 18, 2024

Choose a reason for hiding this comment

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

The example should be updated to include a vertical-align section.

Ideally we would include the images from the design proposal.

If that is difficult for some reason, please update the design proposal with the latest modifications (i.e. use bottom instead of baseline and link to it so people have some additional context and visuals.

Choose a reason for hiding this comment

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

I have updated the example:
Screenshot 2024-12-23 at 07 23 35

I also referenced design proposal. It is updated to use bottom instead of baseline.
Screenshot 2024-12-23 at 07 23 27

For the images - technically I can do it (it works) however there are no images in Expression documentation at all. I'm not sure what is a pattern here. Also I think it is good that expressions documentation is concise, and I'm not sure if adding images won't change that. I think it will be the most valuable to add or update example after maplibre-gl-js implementation. What do you think?

"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 }", "..."],
HarelM marked this conversation as resolved.
Show resolved Hide resolved
"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
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
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
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
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"
}
]
}
]
}
}
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
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