Add 29 example layouts for the v0.9 basic catalog#761
Add 29 example layouts for the v0.9 basic catalog#761jacobsimionato merged 1 commit intogoogle:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request is a great initiative to add a comprehensive set of 29 example layouts for the v0.9 basic catalog, which will be very valuable for integration and testing. Overall, the structure of the examples is good. However, I've identified a few critical issues and areas for improvement across the new files:
- Schema Violations: There are widespread schema violations, particularly with invalid icon names and the incorrect use of the
actionproperty onTextFieldcomponents. These are critical as they will cause rendering failures. - Security: The login form example has a security flaw where the password field is not obscured.
- Maintainability & Scalability: Several examples use hardcoded lists of components instead of the more robust and scalable templating feature. This makes them brittle to changes in the data model.
- Clarity: In one case, a component ID is misleading, which impacts code clarity.
I've provided specific comments and suggestions to address these points. Applying these changes will significantly improve the quality and correctness of these examples.
Note: Security Review has been skipped due to the limited scope of the PR.
| { | ||
| "id": "flight-indicator", | ||
| "component": "Icon", | ||
| "name": "flight" | ||
| }, |
There was a problem hiding this comment.
| { | ||
| "id": "email-field", | ||
| "component": "TextField", | ||
| "value": { | ||
| "path": "/email" | ||
| }, | ||
| "placeholder": "Email address", | ||
| "label": "Email", | ||
| "action": { | ||
| "event": { | ||
| "name": "updateEmail", | ||
| "context": {} | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
The action property is not valid for a TextField component according to the basic_catalog.json schema. Input changes are handled via two-way data binding, and action is typically reserved for components like Button that trigger a specific event.
{
"id": "email-field",
"component": "TextField",
"value": {
"path": "/email"
},
"placeholder": "Email address",
"label": "Email"
},| { | ||
| "id": "password-field", | ||
| "component": "TextField", | ||
| "value": { | ||
| "path": "/password" | ||
| }, | ||
| "placeholder": "Password", | ||
| "label": "Password", | ||
| "action": { | ||
| "event": { | ||
| "name": "updatePassword", | ||
| "context": {} | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
The password-field component has two issues:
- It's a
TextFieldfor a password but doesn't use theobscuredvariant. This will cause the password to be displayed in plain text as the user types, which is a security vulnerability. - The
actionproperty is not valid for aTextFieldcomponent according to thebasic_catalog.jsonschema. Input changes are handled via two-way data binding, andactionis typically reserved for components likeButton.
{
"id": "password-field",
"component": "TextField",
"value": {
"path": "/password"
},
"placeholder": "Password",
"label": "Password",
"variant": "obscured"
},| { | ||
| "id": "location-icon", | ||
| "component": "Icon", | ||
| "name": "location_on" | ||
| }, |
There was a problem hiding this comment.
The icon name "location_on" is not valid. It seems you are using snake_case, but the basic_catalog.json schema specifies icon names in camelCase. The correct name is locationOn. This casing issue is present for other icons in the examples as well.
{
"id": "location-icon",
"component": "Icon",
"name": "locationOn"
}| { | ||
| "id": "forecast-row", | ||
| "component": "Row", | ||
| "children": [ | ||
| "day1", | ||
| "day2", | ||
| "day3", | ||
| "day4", | ||
| "day5" | ||
| ], | ||
| "justify": "spaceAround" | ||
| }, | ||
| { | ||
| "id": "day1", | ||
| "component": "Column", | ||
| "children": [ | ||
| "day1-icon", | ||
| "day1-temp" | ||
| ], | ||
| "align": "center" | ||
| }, | ||
| { | ||
| "id": "day1-icon", | ||
| "component": "Text", | ||
| "text": { | ||
| "path": "/forecast/0/icon" | ||
| }, | ||
| "variant": "h3" | ||
| }, | ||
| { | ||
| "id": "day1-temp", | ||
| "component": "Text", | ||
| "text": { | ||
| "path": "/forecast/0/temp" | ||
| }, | ||
| "variant": "caption" | ||
| }, | ||
| { | ||
| "id": "day2", | ||
| "component": "Column", | ||
| "children": [ | ||
| "day2-icon", | ||
| "day2-temp" | ||
| ], | ||
| "align": "center" | ||
| }, | ||
| { | ||
| "id": "day2-icon", | ||
| "component": "Text", | ||
| "text": { | ||
| "path": "/forecast/1/icon" | ||
| }, | ||
| "variant": "h3" | ||
| }, | ||
| { | ||
| "id": "day2-temp", | ||
| "component": "Text", | ||
| "text": { | ||
| "path": "/forecast/1/temp" | ||
| }, | ||
| "variant": "caption" | ||
| }, | ||
| { | ||
| "id": "day3", | ||
| "component": "Column", | ||
| "children": [ | ||
| "day3-icon", | ||
| "day3-temp" | ||
| ], | ||
| "align": "center" | ||
| }, | ||
| { | ||
| "id": "day3-icon", | ||
| "component": "Text", | ||
| "text": { | ||
| "path": "/forecast/2/icon" | ||
| }, | ||
| "variant": "h3" | ||
| }, | ||
| { | ||
| "id": "day3-temp", | ||
| "component": "Text", | ||
| "text": { | ||
| "path": "/forecast/2/temp" | ||
| }, | ||
| "variant": "caption" | ||
| }, | ||
| { | ||
| "id": "day4", | ||
| "component": "Column", | ||
| "children": [ | ||
| "day4-icon", | ||
| "day4-temp" | ||
| ], | ||
| "align": "center" | ||
| }, | ||
| { | ||
| "id": "day4-icon", | ||
| "component": "Text", | ||
| "text": { | ||
| "path": "/forecast/3/icon" | ||
| }, | ||
| "variant": "h3" | ||
| }, | ||
| { | ||
| "id": "day4-temp", | ||
| "component": "Text", | ||
| "text": { | ||
| "path": "/forecast/3/temp" | ||
| }, | ||
| "variant": "caption" | ||
| }, | ||
| { | ||
| "id": "day5", | ||
| "component": "Column", | ||
| "children": [ | ||
| "day5-icon", | ||
| "day5-temp" | ||
| ], | ||
| "align": "center" | ||
| }, | ||
| { | ||
| "id": "day5-icon", | ||
| "component": "Text", | ||
| "text": { | ||
| "path": "/forecast/4/icon" | ||
| }, | ||
| "variant": "h3" | ||
| }, | ||
| { | ||
| "id": "day5-temp", | ||
| "component": "Text", | ||
| "text": { | ||
| "path": "/forecast/4/temp" | ||
| }, | ||
| "variant": "caption" | ||
| } | ||
| ] |
There was a problem hiding this comment.
The forecast-row component and its children (day1 through day5) are hardcoded to display a five-day forecast. This implementation is not scalable and will break or render incorrectly if the forecast data array has a different number of items.
A better approach is to use the dynamic list templating feature. This involves:
- Defining a single template component for a forecast day.
- Binding the
childrenproperty offorecast-rowto the/forecastdata array, referencing the template. - Using relative paths within the template to bind to item properties like
iconandtemp.
This makes the layout robust and adaptable to data of any length.
{
"id": "forecast-row",
"component": "Row",
"children": {
"path": "/forecast",
"componentId": "forecast-day-template"
},
"justify": "spaceAround"
},
{
"id": "forecast-day-template",
"component": "Column",
"children": [
"day-icon",
"day-temp"
],
"align": "center"
},
{
"id": "day-icon",
"component": "Text",
"text": {
"path": "icon"
},
"variant": "h3"
},
{
"id": "day-temp",
"component": "Text",
"text": {
"path": "temp"
},
"variant": "caption"
}| { | ||
| "id": "events-list", | ||
| "component": "Divider" | ||
| }, |
There was a problem hiding this comment.
The component with id: "events-list" is a Divider. This name is misleading as it suggests a list of events, while the component is just a visual separator. For better clarity and maintainability, the ID should reflect its purpose (e.g., divider). Please also update the reference in the children array of the main-column component.
{
"id": "divider",
"component": "Divider"
}
Description
This PR adds 29 example layouts to the v0.9
basic_catalogspecification. The layouts were adapted from the gallery source code attools/composer/src/data/gallery/**into the v0.9server_to_client_list.jsonformat.Key mappings handled during the transformation:
ProgressBarwithSlideralignmenttoalign,distributiontojustify, andusageHinttovariantbaselinealignments to the supportedstartenumaction: 'name'strings into object structures{"event": {"name": "...", "context": {}}}These example files provide integration scenarios and test beds for implementations of the v0.9
basic_catalog.Pre-launch Checklist