Fix icons and modals in 0.8, support 0.8 in the angular explorer app and add missing 0.8 integration tests#1405
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 introduces multi-version support (v0.8 and v0.9) to the A2UI Explorer, enabling users to toggle between versions via a new UI selector. The changes include refactoring the example generation script to bundle messages for both versions, updating the AgentStubService and DemoComponent to handle version-specific message processing, and enhancing v0.8 components such as the Modal and Icon. Feedback was provided regarding a bug in the Icon component's name resolution logic, where the regex conversion to snake_case incorrectly prepends an underscore to leading uppercase characters.
josemontespg
changed the title
josemontespg
changed the title
|
For your future PRs: try to separate code reorg and code changes (make two different PRs), to make it easier to make reviews. |
|
Would you mind to link an issue that it contributes to, in the description? (if you did not know that PR can auto-resolve an issue: flutter/genui#914) |
Sorry for the large change, I'll keep this in mind in the future. Thank you for taking the time to review it!
Added a link to the internal issue in the description that motivates this PR |
polina-c
left a comment
polina-c
left a comment
There was a problem hiding this comment.
LGTM, but I would want also review from someone who is more familiar with our ts codebase.
ditman
left a comment
ditman
left a comment
There was a problem hiding this comment.
I am hesitant of adding v0.8 "inline" to the explorer app; I think this should be a slightly different "demo" component.
Also, needs CHANGELOG updates.
Also also, @polina-c is right, can we please split this PR in at least 3 or 4 (support angular explorer app 0.8, add missing tests, fix icons, fix modals?)
| if (!example) { | ||
| example = examples.find(ex => ex.name === `${exampleName} (minimal)`); | ||
| } |
There was a problem hiding this comment.
I think we should remove the "minimal" catalog and examples. They don't add much value once the renderer is fully built (instead, they add maintenance burden like here)
There was a problem hiding this comment.
Sounds good to me. Is there alignment on doing this? Created #1432 to track
| examples = EXAMPLES; | ||
| selectedExample: Example | undefined = undefined; | ||
| version: '0.8' | '0.9' = '0.9'; | ||
| examples: Array<Example | Example_08> = EXAMPLES_V09; |
There was a problem hiding this comment.
Would it make sense to have DemoComponentV08 and DemoComponentV09 where each one of them knows how to handle its version of the protocol (with duplicated code) instead of adding many "if version == 0.8 else..." here?
|
Thanks for the feedback! I'm closing this PR in favor of smaller PRs |
3 participants
This PR Fixes the modal and icon components in the 0.8 basic catalog. Also implements a dropdown in the Angular explorer sample app to switch between v0.8 and v0.9 specifications and renderers, to help with manual and automated testing.
Key changes:
?version=0.8URL parameter to load a specific version directly.generate-examples.mjsto bundle examples for both versions.Why:
The motivation for these tests is http://b/511068435#comment3 because we were missing a test for the 0.8 modal component. I found that we were missing 0.8 tests for the explorer app, which is already a great sample app to exercise many different features and edge cases for 0.8.
Testing: