Add `vertical-align` in format expression #900

stanislawpuda-tomtom · 2024-11-13T11:49:55Z

Related issue #832.

This PR introduces vertical align property to format expression. It enables to specify how each section should be positioned in relation to biggest element in line. There are three possible options:

"bottom" default: text baseline or image bottom are in line - this is current behaviour.
"center": image center or text center are in line.
"top": image top and text top are in line

Launch Checklist

Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
Briefly describe the changes in this PR.
Link to related issues.
Include before/after visuals or gifs if this PR includes visual changes.
Write tests for all new functionality.
Document any changes to public APIs.
Post benchmark scores.
Add an entry to CHANGELOG.md under the ## main section.

…-spec into issue-832-format-expression-vertical-align

codecov-commenter · 2024-11-13T11:51:15Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.80%. Comparing base (c6fd205) to head (37e1178).
Report is 11 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #900      +/-   ##
==========================================
+ Coverage   92.78%   92.80%   +0.01%     
==========================================
  Files         107      107              
  Lines        4739     4752      +13     
  Branches     1346     1352       +6     
==========================================
+ Hits         4397     4410      +13     
  Misses        342      342

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…-spec into issue-832-format-expression-vertical-align

stanislawpuda-tomtom · 2024-11-27T11:55:13Z

I would like to cover questions from the meeting on Nov 13th:

Using HTML

I did check, and it is possible to use HTML to display vertically aligned labels. This approach works for MapLibre GL JS, however it does not cover multiple platforms. There are more advantages of using format expression over custom HTML layer:

much easier to use
HTML markers don't support collision detection, as far as I know, which makes the implementation even more difficult
better performance
defined once in the style spec for multiple platforms

Format expression already gives possibility to create rich labels on the map, easily, without having to implement own custom solution. Having the possibility to align elements vertically in my opinion is good supplement to format expression. In some cases necessary to make full benefit from it.

Sample use case

Our team created a sample mock how the labels look like with and without vertical alignment.

Before:

After:

lseelenbinder · 2024-11-27T14:27:56Z

Thanks for taking the time to work through these pieces @stanislawpuda-tomtom!

In my opinion, those advantages are sufficient to merit including in the spec—especially for collision detection and performance reasons.

stanislawpuda-tomtom · 2024-11-28T09:10:29Z

I have changed baseline to bottom.
I also modified one test to check if vertical-align is recognised for image section.

src/reference/v8.json

HarelM · 2024-11-28T09:21:49Z

Are you sure about the places where you added the code?
It seems that equivalent format related code does not look the same.
I know this is an enum while the others are string, or number, but I find the code a bit weird...

stanislawpuda-tomtom · 2024-11-28T10:59:51Z

I've added vertical-align to SDK support table.

For the code, let me try to explain:

let verticalAlign = null;
if (arg['vertical-align']) { 
    // covers case, when `vertical-align` is defined with plain string, then we can check if matches enum
    if (typeof arg['vertical-align'] === 'string' && !VERTICAL_ALIGN_OPTIONS.includes(arg['vertical-align'] as VerticalAlign)) {
        return context.error(`\`vertical-align\` must be one of: 'bottom', 'center', 'top' but found '${arg['vertical-align']}' instead.`) as null;
    }

    // in this case it returns expression, so we can't check if it matches enum, only if it is string
    verticalAlign = context.parse(arg['vertical-align'], 1, StringType);
    if (!verticalAlign) return null;
}

src/expression/definitions/format.ts

src/reference/v8.json

stanislawpuda-tomtom · 2024-12-09T08:42:09Z

@HarelM @lseelenbinder
Is it possible to proceed with this PR? Is there something I can do, clarify, or is there any blocker?

HarelM · 2024-12-09T11:30:55Z

src/expression/definitions/format.ts

@@ -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)) {


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

HarelM · 2024-12-09T11:32:25Z

src/expression/definitions/format.ts

+                    null,
+                    null,
+                    null,
+                    section.verticalAlign ? section.verticalAlign.evaluate(ctx) : null


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

HarelM · 2024-12-09T11:33:38Z

src/expression/types/formatted.ts

@@ -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;


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

HarelM · 2024-12-09T11:35:49Z

src/reference/v8.json

        "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 }", "..."],


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

HarelM · 2024-12-09T11:37:28Z

I've added details on my concerns regarding the current code. Let me know if this is clearer now.

stanislawpuda-tomtom · 2024-12-10T12:49:08Z

@HarelM It is clear - I'll cover them. Thank you!

stanislawpuda-tomtom added 7 commits October 2, 2024 11:32

Add vertical-align to v8.json

d785d2b

Add vertical-align to format definition

cfd9111

Add static value validation

7593fa9

Add support for image

6065c88

update vertical-align description

a4630d0

Merge branch 'main' of https://github.com/tomtom-forks/maplibre-style…

071c277

…-spec into issue-832-format-expression-vertical-align

add changelog

1a5cf7c

stanislawpuda-tomtom mentioned this pull request Nov 13, 2024

Vertical alignment in format expression maplibre/maplibre-gl-js#5043

Draft

8 tasks

Merge branch 'main' of https://github.com/tomtom-forks/maplibre-style…

4460a7e

…-spec into issue-832-format-expression-vertical-align

stanislawpuda-tomtom added 2 commits November 28, 2024 09:54

add image section vertical-align test

695efdf

change baseline to bottom

c74afe5

stanislawpuda-tomtom mentioned this pull request Nov 28, 2024

Vertical alignment in format expression #832

Open

HarelM reviewed Nov 28, 2024

View reviewed changes

src/reference/v8.json Show resolved Hide resolved

stanislawpuda-tomtom added 2 commits November 28, 2024 11:38

add vertical-align to sdk support table

b55daaa

add links to maplibre native issue

2429fe8

zbigniewmatysek-tomtom reviewed Nov 28, 2024

View reviewed changes

src/expression/definitions/format.ts Outdated Show resolved Hide resolved

src/reference/v8.json Outdated Show resolved Hide resolved

stanislawpuda-tomtom and others added 3 commits November 28, 2024 15:10

review fixes

4cf94a5

Merge branch 'main' into issue-832-format-expression-vertical-align

8e957bd

Merge branch 'main' into issue-832-format-expression-vertical-align

37e1178

HarelM reviewed Dec 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `vertical-align` in format expression #900

Add `vertical-align` in format expression #900

stanislawpuda-tomtom commented Nov 13, 2024 •

edited

Loading

codecov-commenter commented Nov 13, 2024 •

edited

Loading

stanislawpuda-tomtom commented Nov 27, 2024

lseelenbinder commented Nov 27, 2024

stanislawpuda-tomtom commented Nov 28, 2024

HarelM commented Nov 28, 2024

stanislawpuda-tomtom commented Nov 28, 2024 •

edited

Loading

stanislawpuda-tomtom commented Dec 9, 2024

HarelM Dec 9, 2024

HarelM Dec 9, 2024

HarelM Dec 9, 2024

HarelM Dec 9, 2024

HarelM commented Dec 9, 2024

stanislawpuda-tomtom commented Dec 10, 2024

Add vertical-align in format expression #900

Are you sure you want to change the base?

Add vertical-align in format expression #900

Conversation

stanislawpuda-tomtom commented Nov 13, 2024 • edited Loading

Launch Checklist

codecov-commenter commented Nov 13, 2024 • edited Loading

Codecov Report

stanislawpuda-tomtom commented Nov 27, 2024

Using HTML

Sample use case

lseelenbinder commented Nov 27, 2024

stanislawpuda-tomtom commented Nov 28, 2024

HarelM commented Nov 28, 2024

stanislawpuda-tomtom commented Nov 28, 2024 • edited Loading

stanislawpuda-tomtom commented Dec 9, 2024

HarelM Dec 9, 2024

Choose a reason for hiding this comment

HarelM Dec 9, 2024

Choose a reason for hiding this comment

HarelM Dec 9, 2024

Choose a reason for hiding this comment

HarelM Dec 9, 2024

Choose a reason for hiding this comment

HarelM commented Dec 9, 2024

stanislawpuda-tomtom commented Dec 10, 2024

Add `vertical-align` in format expression #900

Add `vertical-align` in format expression #900

stanislawpuda-tomtom commented Nov 13, 2024 •

edited

Loading

codecov-commenter commented Nov 13, 2024 •

edited

Loading

stanislawpuda-tomtom commented Nov 28, 2024 •

edited

Loading