-
Notifications
You must be signed in to change notification settings - Fork 1.1k
refactor(web_core): centralize layout mapping logic #1258
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
Open
ppazosp
wants to merge
6
commits into
google:main
Choose a base branch
from
ppazosp:refactor/840-centralize-layout-mapping
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
531b5f8
refactor[web_core]: centralize layout mapping logic (fixes #840)
ppazosp 470fe86
refactor[web_core]: use object maps for layout + add tests per review
ppazosp 1416578
refactor[web_core]: move layout helpers to basic_catalog/styles per r…
ppazosp 238a34e
Merge upstream/main into refactor/840-centralize-layout-mapping
ppazosp 45224e8
fix[web_core]: preserve pass-through and nullish semantics in layout …
ppazosp 850e87e
fix[web_core]: harden layout maps per gemini review
ppazosp File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
92 changes: 92 additions & 0 deletions
92
renderers/web_core/src/v0_9/basic_catalog/styles/layout.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,92 @@ | ||
| /* | ||
| * Copyright 2025 Google LLC | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * https://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| import {describe, it} from 'node:test'; | ||
| import * as assert from 'node:assert'; | ||
|
|
||
| import {mapAlign, mapJustify} from './layout.js'; | ||
|
|
||
| describe('mapJustify', () => { | ||
| it('maps center to center', () => { | ||
| assert.strictEqual(mapJustify('center'), 'center'); | ||
| }); | ||
| it('maps end to flex-end', () => { | ||
| assert.strictEqual(mapJustify('end'), 'flex-end'); | ||
| }); | ||
| it('maps spaceAround to space-around', () => { | ||
| assert.strictEqual(mapJustify('spaceAround'), 'space-around'); | ||
| }); | ||
| it('maps spaceBetween to space-between', () => { | ||
| assert.strictEqual(mapJustify('spaceBetween'), 'space-between'); | ||
| }); | ||
| it('maps spaceEvenly to space-evenly', () => { | ||
| assert.strictEqual(mapJustify('spaceEvenly'), 'space-evenly'); | ||
| }); | ||
| it('maps start to flex-start', () => { | ||
| assert.strictEqual(mapJustify('start'), 'flex-start'); | ||
| }); | ||
| it('maps stretch to stretch', () => { | ||
| assert.strictEqual(mapJustify('stretch'), 'stretch'); | ||
| }); | ||
| it('returns undefined for undefined input so consumers leave the style unset', () => { | ||
| assert.strictEqual(mapJustify(undefined), undefined); | ||
| }); | ||
| it('returns undefined for null input', () => { | ||
| assert.strictEqual(mapJustify(null), undefined); | ||
| }); | ||
| it('passes through unknown values unchanged', () => { | ||
| assert.strictEqual(mapJustify('unknown'), 'unknown'); | ||
| }); | ||
| it('does not resolve Object.prototype keys such as toString', () => { | ||
| assert.strictEqual(mapJustify('toString'), 'toString'); | ||
| }); | ||
| it('passes through empty string unchanged', () => { | ||
| assert.strictEqual(mapJustify(''), ''); | ||
| }); | ||
| }); | ||
|
|
||
| describe('mapAlign', () => { | ||
| it('maps center to center', () => { | ||
| assert.strictEqual(mapAlign('center'), 'center'); | ||
| }); | ||
| it('maps end to flex-end', () => { | ||
| assert.strictEqual(mapAlign('end'), 'flex-end'); | ||
| }); | ||
| it('maps start to flex-start', () => { | ||
| assert.strictEqual(mapAlign('start'), 'flex-start'); | ||
| }); | ||
| it('maps stretch to stretch', () => { | ||
| assert.strictEqual(mapAlign('stretch'), 'stretch'); | ||
| }); | ||
| it('maps baseline to baseline', () => { | ||
| assert.strictEqual(mapAlign('baseline'), 'baseline'); | ||
| }); | ||
| it('returns undefined for undefined input so consumers leave the style unset', () => { | ||
| assert.strictEqual(mapAlign(undefined), undefined); | ||
| }); | ||
| it('returns undefined for null input', () => { | ||
| assert.strictEqual(mapAlign(null), undefined); | ||
| }); | ||
| it('passes through unknown values unchanged', () => { | ||
| assert.strictEqual(mapAlign('unknown'), 'unknown'); | ||
| }); | ||
| it('does not resolve Object.prototype keys such as toString', () => { | ||
| assert.strictEqual(mapAlign('toString'), 'toString'); | ||
| }); | ||
| it('passes through empty string unchanged', () => { | ||
| assert.strictEqual(mapAlign(''), ''); | ||
| }); | ||
| }); |
75 changes: 75 additions & 0 deletions
75
renderers/web_core/src/v0_9/basic_catalog/styles/layout.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| /* | ||
| * Copyright 2025 Google LLC | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * https://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| /** | ||
| * Centralized layout mapping utilities for Web/CSS-based renderers. | ||
| * | ||
| * Maps A2UI layout enum values (e.g., `spaceBetween`) to their corresponding | ||
| * CSS values (e.g., `space-between`). These functions are shared across all | ||
| * web renderers (React, Lit, Angular) to ensure consistent behavior. | ||
| * | ||
| * Contract: nullish input returns `undefined` so consumers leave the CSS | ||
| * property unset (inherits from cascade / CSS variables). Unknown keys are | ||
| * passed through as-is so that future enum additions or spec mismatches | ||
| * remain visible to the browser rather than silently coerced to a default. | ||
| * | ||
| * The maps use `Map` rather than plain objects to avoid prototype-chain | ||
| * lookups (e.g. `mapJustify('toString')` must not hit `Object.prototype`). | ||
| */ | ||
|
|
||
| const justifyMap = new Map<string, string>([ | ||
| ['center', 'center'], | ||
| ['end', 'flex-end'], | ||
| ['spaceAround', 'space-around'], | ||
| ['spaceBetween', 'space-between'], | ||
| ['spaceEvenly', 'space-evenly'], | ||
| ['start', 'flex-start'], | ||
| ['stretch', 'stretch'], | ||
| ]); | ||
|
|
||
| /** | ||
| * Maps an A2UI justify enum value to its CSS `justify-content` equivalent. | ||
| * | ||
| * @param value - An A2UI justify value such as `'start'`, `'center'`, | ||
| * `'spaceBetween'`, etc. | ||
| * @returns The mapped CSS value, the raw input if the key is unknown, or | ||
| * `undefined` if the input is nullish. | ||
| */ | ||
| export function mapJustify(value?: string | null): string | undefined { | ||
| if (value === undefined || value === null) return undefined; | ||
| return justifyMap.get(value) ?? value; | ||
| } | ||
|
|
||
| const alignMap = new Map<string, string>([ | ||
| ['baseline', 'baseline'], | ||
| ['center', 'center'], | ||
| ['end', 'flex-end'], | ||
| ['start', 'flex-start'], | ||
| ['stretch', 'stretch'], | ||
| ]); | ||
|
|
||
| /** | ||
| * Maps an A2UI align enum value to its CSS `align-items` equivalent. | ||
| * | ||
| * @param value - An A2UI align value such as `'start'`, `'center'`, `'end'`, | ||
| * `'stretch'`, or `'baseline'`. | ||
| * @returns The mapped CSS value, the raw input if the key is unknown, or | ||
| * `undefined` if the input is nullish. | ||
| */ | ||
| export function mapAlign(value?: string | null): string | undefined { | ||
| if (value === undefined || value === null) return undefined; | ||
| return alignMap.get(value) ?? value; | ||
| } | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a
Mapis a good choice to avoid prototype chain lookups. To further improve type safety and prevent accidental mutations, consider marking the maps asReadonlyMapand potentially using a union type for the keys if the A2UI layout enums are defined elsewhere in the codebase.