Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a centralized theme injection system for A2UI components, adding a default style utility for global CSS variables and an injection function. Components across the basic and minimal catalogs are updated to use these variables and call the injection function in their lifecycle. Feedback identifies a missing theme injection in the basic Text component, suggests using internal CSS variables for better color inheritance in buttons, and recommends guarding document access to ensure compatibility with server-side rendering.
| */ | ||
|
|
||
| import { html, nothing } from "lit"; | ||
| import { html, nothing, css } from "lit"; |
There was a problem hiding this comment.
The injectDefaultA2uiTheme utility is missing from the imports in this file, which is inconsistent with the other components updated in this pull request.
| import { html, nothing, css } from "lit"; | |
| import { html, nothing, css } from "lit"; | |
| import { injectDefaultA2uiTheme } from "@a2ui/web_core/v0_9"; |
| static styles = css` | ||
| p, h1, h2, h3, h4, h5, h6 { | ||
| margin: var(--_a2ui-text-margin, revert); | ||
| } | ||
| `; |
There was a problem hiding this comment.
This component is missing the connectedCallback to inject the default A2UI theme. To ensure consistent styling across all widgets, it should follow the same pattern as the other components in this PR.
| static styles = css` | |
| p, h1, h2, h3, h4, h5, h6 { | |
| margin: var(--_a2ui-text-margin, revert); | |
| } | |
| `; | |
| static styles = css` | |
| p, h1, h2, h3, h4, h5, h6 { | |
| margin: var(--_a2ui-text-margin, revert); | |
| } | |
| `; | |
| connectedCallback() { | |
| super.connectedCallback(); | |
| injectDefaultA2uiTheme(); | |
| } |
| .a2ui-button { | ||
| --_a2ui-text-margin: 0; | ||
| padding: var(--_button-padding); | ||
| background-color: var(--a2ui-color-secondary, #ddd); | ||
| color: var(--a2ui-color-on-secondary, #333); | ||
| border: var(--_button-border); | ||
| border-radius: var(--_button-border-radius); | ||
| cursor: pointer; | ||
| display: inline-flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
| } | ||
| .a2ui-button.a2ui-button-primary { | ||
| border-color: var(--_color-primary); | ||
| background-color: var(--_color-primary); | ||
| color: var(--a2ui-color-on-primary, #fff); | ||
| } |
There was a problem hiding this comment.
The basic Button component is missing the --_a2ui-text-color variable definition used in the minimal version. Adding this ensures that nested components (like Text) inherit the correct foreground color based on the button's variant.
| .a2ui-button { | |
| --_a2ui-text-margin: 0; | |
| padding: var(--_button-padding); | |
| background-color: var(--a2ui-color-secondary, #ddd); | |
| color: var(--a2ui-color-on-secondary, #333); | |
| border: var(--_button-border); | |
| border-radius: var(--_button-border-radius); | |
| cursor: pointer; | |
| display: inline-flex; | |
| align-items: center; | |
| justify-content: center; | |
| } | |
| .a2ui-button.a2ui-button-primary { | |
| border-color: var(--_color-primary); | |
| background-color: var(--_color-primary); | |
| color: var(--a2ui-color-on-primary, #fff); | |
| } | |
| .a2ui-button { | |
| --_a2ui-text-margin: 0; | |
| --_a2ui-text-color: var(--a2ui-color-on-secondary, #333); | |
| padding: var(--_button-padding); | |
| background-color: var(--a2ui-color-secondary, #ddd); | |
| color: var(--_a2ui-text-color); | |
| border: var(--_button-border); | |
| border-radius: var(--_button-border-radius); | |
| cursor: pointer; | |
| display: inline-flex; | |
| align-items: center; | |
| justify-content: center; | |
| } | |
| .a2ui-button.a2ui-button-primary { | |
| --_a2ui-text-color: var(--a2ui-color-on-primary, #fff); | |
| border-color: var(--_color-primary); | |
| background-color: var(--_color-primary); | |
| color: var(--_a2ui-text-color); | |
| } |
| .a2ui-button { | ||
| --_a2ui-text-margin: 0; | ||
| --_a2ui-text-color: var(--a2ui-color-on-secondary, #333); | ||
| padding: var(--_button-padding); | ||
| background-color: var(--a2ui-color-secondary, #ddd); | ||
| color: var(--a2ui-color-on-secondary, #333); | ||
| border: var(--_button-border); | ||
| border-radius: var(--_button-border-radius); | ||
| cursor: pointer; | ||
| display: inline-flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
| } | ||
| .a2ui-button.a2ui-button-primary { | ||
| --_a2ui-text-color: var(--a2ui-color-on-primary, #fff); | ||
| border-color: var(--_color-primary); | ||
| background-color: var(--_color-primary); | ||
| color: var(--a2ui-color-on-primary, #fff); | ||
| } |
There was a problem hiding this comment.
For better maintainability and consistency, the button's color property should use the internal --_a2ui-text-color variable that was just defined, rather than repeating the global variable lookup.
| .a2ui-button { | |
| --_a2ui-text-margin: 0; | |
| --_a2ui-text-color: var(--a2ui-color-on-secondary, #333); | |
| padding: var(--_button-padding); | |
| background-color: var(--a2ui-color-secondary, #ddd); | |
| color: var(--a2ui-color-on-secondary, #333); | |
| border: var(--_button-border); | |
| border-radius: var(--_button-border-radius); | |
| cursor: pointer; | |
| display: inline-flex; | |
| align-items: center; | |
| justify-content: center; | |
| } | |
| .a2ui-button.a2ui-button-primary { | |
| --_a2ui-text-color: var(--a2ui-color-on-primary, #fff); | |
| border-color: var(--_color-primary); | |
| background-color: var(--_color-primary); | |
| color: var(--a2ui-color-on-primary, #fff); | |
| } | |
| .a2ui-button { | |
| --_a2ui-text-margin: 0; | |
| --_a2ui-text-color: var(--a2ui-color-on-secondary, #333); | |
| padding: var(--_button-padding); | |
| background-color: var(--a2ui-color-secondary, #ddd); | |
| color: var(--_a2ui-text-color); | |
| border: var(--_button-border); | |
| border-radius: var(--_button-border-radius); | |
| cursor: pointer; | |
| display: inline-flex; | |
| align-items: center; | |
| justify-content: center; | |
| } | |
| .a2ui-button.a2ui-button-primary { | |
| --_a2ui-text-color: var(--a2ui-color-on-primary, #fff); | |
| border-color: var(--_color-primary); | |
| background-color: var(--_color-primary); | |
| color: var(--_a2ui-text-color); | |
| } |
| * This method ensures the default stylesheet is added to the root document | ||
| * of the page, if it's not already present. | ||
| */ | ||
| export function injectDefaultA2uiTheme() { |
There was a problem hiding this comment.
| this.style.flex = | ||
| props.weight !== undefined ? String(props.weight) : "initial"; | ||
| this.style.justifyContent = mapJustify(props.justify); | ||
| this.style.alignItems = mapAlign(props.align); |
There was a problem hiding this comment.
I wonder if it's better to leave the old styleMap object instead of mutating the styles directly on updated
There was a problem hiding this comment.
Can we move this to within https://github.com/google/A2UI/tree/main/renderers/web_core/src/v0_9/basic_catalog/styles to show that it's just a convenience set of styles that you could use with the basic catalog, but really not a top-level part of the A2UI specification? Or else leave it where it is, but label it "basic" or "example" or something?
6f17781 to
00d299c
Compare
The function allows basic catalog implementations to inject a shared stylesheet that contains a set of overridable variables used by the widgets to keep the styling somewhat consistent.
00d299c to
903d6c6
Compare
Description
This PR continues #1057 by applying the same pattern to the rest of the catalog elements of the Lit renderer.
Pre-launch Checklist
If you need help, consider asking for advice on the discussion board.