[lit] Wire A2UI primaryColor on basic catalog elements.#1343
[lit] Wire A2UI primaryColor on basic catalog elements.#1343ditman wants to merge 10 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 implements support for agent-provided primary colors across basic catalog components. Key changes include the addition of a computeColor utility in web_core to generate CSS formulas for color variants and the integration of these styles into the Lit renderer. The explorer tool was also updated with a color picker for live testing. Review feedback recommends adding type safety checks when processing messages to avoid potential runtime errors and optimizing the Lit component lifecycle by moving style property updates to willUpdate to minimize repaints and avoid flashes of unstyled content.
ditman
left a comment
ditman
left a comment
There was a problem hiding this comment.
I left some comments/questions for myself and the reviewers!
| this.style.removeProperty('--a2ui-color-primary-light'); | ||
| this.style.removeProperty('--a2ui-color-primary-dark'); | ||
| this.style.removeProperty('--a2ui-color-primary-hover'); |
There was a problem hiding this comment.
Now that we can compute light/dark/hover programmatically, would it make sense to remove them from the core stylesheet, and let the components that need light/dark/hover variants of a color use the formulas inline with per-widget overrides? That would simplify this a lot, and make the set of CSS variables exposed from web_core smaller. WDYT @josemontespg?
There was a problem hiding this comment.
Sounds good to me. Would that affect React also?
There was a problem hiding this comment.
@josemontespg once we land the formulas to compute light/dark/hover modes for the colors, we'd go into each basic catalog implementation and use that based on the defined --a2ui-xxx-color variable, and remove the usages of the specific "hover"/"dark"/"light" variants. Then we can remove the variables from the core css :)
| * @param options Options containing the dark and light variant variable names. | ||
| * @returns The CSS formula string. | ||
| */ | ||
| export function computeColor(type: 'hover', options: ComputeColorHoverOptions): string; |
There was a problem hiding this comment.
What do you think about renaming this to computeHoverColor to be more clear about what it's used for?
There was a problem hiding this comment.
(I need this to be computeColor across all 3 aliases so I can only export a single function to end users, I didn't want to bloat the public API with color-related methods)
There was a problem hiding this comment.
But....it really is multiple APIs "hiding" behind a single function :-)
Using explicitly named functions would seem to reduce cognitive load on developers.
Unless you're expecting to grow the list of acceptable values for the type parameter over time?
| * @param options Options containing the base color variable and optional percentage. | ||
| * @returns The CSS formula string. | ||
| */ | ||
| export function computeColor(type: 'light' | 'dark', options: ComputeColorLightDarkOptions): string; |
There was a problem hiding this comment.
Would it make more sense to have separate computeLightColor and computeDarkColor to have a more explicit API?
There was a problem hiding this comment.
Ah yeah, the way that I wanted this to work is to just export a single function that supports all 3 cases to not bloat the API surface with color-related methods.
All the methods are called the same to leverage TS function overloading so I get some type safety on the "options" types, but they all need to be named "computeColor".
There was a problem hiding this comment.
I renamed this method to computeColorVariant so it's a little bit more clear what it does!
| this.style.removeProperty('--a2ui-color-primary-light'); | ||
| this.style.removeProperty('--a2ui-color-primary-dark'); | ||
| this.style.removeProperty('--a2ui-color-primary-hover'); |
There was a problem hiding this comment.
Sounds good to me. Would that affect React also?
|
OK I did some more tweaks to the Lit explorer app, it looks like this now: 
|
|
Requesting a PTAL from @josemontespg and @jgindin! (thanks for taking a look Jay!) |
…ptions object names.
…dsets to group agent controls.
ditman
force-pushed
the
lit-wire-primarycolor
branch
from
e6e6396 to
f65cd8b
Compare
| } | ||
| } | ||
|
|
||
| advanceMessages(all = false) { |
There was a problem hiding this comment.
I know this code is not changed in this PR, but can we add typing to the all function parameter?
3 participants
Description
This PR modifies the Lit renderer to wire the
theme.primaryColorproperty coming from the A2UI responses that use the basic catalog.There's some minor changes to other packages to make this easier to test:
specification: adds a "primary" button to the basic catalog example 5web_core: adds a mechanism to recompute the "light"/"dark"/"hover" colors using the same formula across the default stylesheet and Lit components later.lit/a2ui_explorer: (optional) adds a way to override thetheme.primaryColorof the sample apps that gets updated in real time as the user changes it.Issues
Pre-launch Checklist
If you need help, consider asking for advice on the discussion board.