refactor(web_core): centralize layout mapping logic#1030
refactor(web_core): centralize layout mapping logic#1030ppazosp wants to merge 2 commits intogoogle:mainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request centralizes the mapJustify and mapAlign layout mapping utilities into the @a2ui/web_core package, removing redundant local implementations across the Angular, Lit, and React renderers. Feedback suggests adding unit tests for these new shared utilities as required by the style guide and considering an object-based mapping approach instead of switch statements to improve maintainability.
| /* | ||
| * 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. | ||
| */ |
There was a problem hiding this comment.
According to the repository style guide (line 17), new code changes should be accompanied by tests. These new utility functions are a core part of the layout logic and should have unit tests to verify their mapping behavior, including handling of default and undefined values.
References
- If there are code changes, code should have tests. (link)
There was a problem hiding this comment.
Done, added 15 unit tests for both functions in layout.test.ts (470fe86).
| export function mapJustify(value?: string): string { | ||
| switch (value) { | ||
| case 'center': | ||
| return 'center'; | ||
| case 'end': | ||
| return 'flex-end'; | ||
| case 'spaceAround': | ||
| return 'space-around'; | ||
| case 'spaceBetween': | ||
| return 'space-between'; | ||
| case 'spaceEvenly': | ||
| return 'space-evenly'; | ||
| case 'start': | ||
| return 'flex-start'; | ||
| case 'stretch': | ||
| return 'stretch'; | ||
| default: | ||
| return 'flex-start'; | ||
| } | ||
| } |
There was a problem hiding this comment.
For improved conciseness and maintainability, you could consider using an object map for this mapping instead of a switch statement. This approach is often more declarative for simple key-value transformations. A similar refactoring can be applied to mapAlign as well.
For example, you could define the maps at the module level and then reference them in the functions:
const justifyMap: Record<string, string> = {
center: 'center',
end: 'flex-end',
spaceAround: 'space-around',
spaceBetween: 'space-between',
spaceEvenly: 'space-evenly',
start: 'flex-start',
stretch: 'stretch',
};
export function mapJustify(value?: string): string {
return (value && justifyMap[value]) || 'flex-start';
}There was a problem hiding this comment.
Done, refactored both functions to use Record<string, string> object maps (470fe86).
1 participant
Description
Creates
mapJustify()andmapAlign()functions inrenderers/web_core/src/v0_9/common/layout.tsas the single source of truth for layout enum-to-CSS mapping.spaceBetween) were passed directly to CSS instead of being mapped to valid values (space-between)Fixes #840
Pre-launch Checklist