Add regex support for textfield on lit renderer#605
Conversation
…ging on-the-wire a2UI JSON and actions
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
… into md-componentslistagent
![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 is a substantial pull request that adds regex validation to text fields, enhances the MultipleChoice component with chip and filterable layouts, and introduces a comprehensive component gallery sample. The new features are well-implemented and the gallery is a valuable addition for demonstration and testing. My review includes suggestions to improve code maintainability, robustness, and style consistency in a few areas, such as simplifying redundant logic, refactoring hardcoded values, and making agent-side dispatching more robust.
| if (!this.processor || !this.component) { | ||
| return Array.isArray(this.selections) ? this.selections : []; | ||
| } | ||
| if (Array.isArray(this.selections)) { | ||
| return this.selections; | ||
| } |
There was a problem hiding this comment.
The logic here can be simplified to be less redundant. The Array.isArray(this.selections) check is effectively performed twice. You can check for the array case first, which covers all scenarios where selections is a literal array, and then handle the data-bound path.
| if (!this.processor || !this.component) { | |
| return Array.isArray(this.selections) ? this.selections : []; | |
| } | |
| if (Array.isArray(this.selections)) { | |
| return this.selections; | |
| } | |
| if (Array.isArray(this.selections)) { | |
| return this.selections; | |
| } | |
| if (!this.processor || !this.component) { | |
| return []; | |
| } |
| import datetime | ||
| import asyncio |
There was a problem hiding this comment.
According to PEP 8, imports should be at the top of the file. Moving import datetime and import asyncio to the top level improves readability and follows standard Python conventions.
References
- PEP 8 recommends that imports are always put at the top of the file, just after any module comments and docstrings, and before module globals and constants. (link)
| } | ||
|
|
||
| .chip.selected:hover { | ||
| background: var(--md-sys-color-secondary-container-high, #e8def8); |
There was a problem hiding this comment.
It's generally better to define CSS variables in a central theme file rather than providing a hardcoded fallback color like #e8def8 directly in the component's CSS. This ensures theming consistency and makes future updates easier. Please define --md-sys-color-secondary-container-high in the appropriate theme file.
| background: var(--md-sys-color-secondary-container-high, #e8def8); | |
| background: var(--md-sys-color-secondary-container-high); |
| if "WHO_ARE_YOU" in query or "START" in query: # Simple trigger for initial load | ||
| gallery_json = get_gallery_json() | ||
| response = f"Here is the component gallery.\n---a2ui_JSON---\n{gallery_json}" | ||
| yield { | ||
| "is_task_complete": True, | ||
| "content": response | ||
| } | ||
| return | ||
|
|
||
| # Handle Actions | ||
| if query.startswith("ACTION:"): |
There was a problem hiding this comment.
The agent's dispatch logic relies on simple string matching (in and startswith), which can be brittle. For instance, an action's context could unintentionally contain the string "ACTION:", leading to incorrect parsing. It would be more robust to parse the incoming request into a structured object first and then dispatch based on its properties. The agent_executor.py already does some of this parsing, and leveraging that more directly would improve robustness.
| const pathMap: Record<string, string> = { | ||
| "demo-text": "galleryData/textField", | ||
| "demo-text-regex": "galleryData/textFieldRegex", | ||
| "demo-checkbox": "galleryData/checkbox", | ||
| "demo-slider": "galleryData/slider", | ||
| "demo-date": "galleryData/date", | ||
| "demo-multichoice": "galleryData/favorites", | ||
| "demo-multichoice-chips": "galleryData/favoritesChips", | ||
| "demo-multichoice-filter": "galleryData/favoritesFilter" | ||
| }; |
There was a problem hiding this comment.
The hardcoded pathMap creates a tight coupling between the client-side component gallery and the agent's data model structure. While this might be acceptable for a self-contained demo, in a real-world scenario this makes the client less reusable and harder to maintain. Consider having the agent provide this mapping or designing the components to receive the full data path as a property, which would make the client more generic.
gspencergoog
left a comment
gspencergoog
left a comment
There was a problem hiding this comment.
This is a rubber stamp: I'm not reviewing the web code, since that's not my specialty.
| id="data" | ||
| .value=${value} | ||
| .placeholder=${"Please enter a value"} | ||
| pattern=${this.validationRegexp || nothing} |
There was a problem hiding this comment.
LGTM on this, but can you separate this change from your other changes? I assume this only needs like a 5 line change in this file?
3 participants
Description
Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.
List which issues are fixed by this PR. For larger changes, raising an issue first helps reduce redundant work.
Pre-launch Checklist
If you need help, consider asking for advice on the discussion board.