-
Notifications
You must be signed in to change notification settings - Fork 825
feat: vue3 renderer and some vue3 samples #591
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a comprehensive Vue 3 renderer for A2UI, complete with core components, a message processor, and sample applications. The implementation is well-structured, leveraging modern Vue 3 features like the Composition API and <script setup>. The separation between rendering logic, data handling, and component definitions is clean. My review focuses on improving robustness, ensuring consistency with the specifications, and enhancing maintainability. I've identified a few areas for improvement, including incomplete feature implementation in an input component and opportunities to make the code more resilient and consistent. Overall, this is a very strong contribution that significantly expands the project's capabilities.
| :id="inputId" | ||
| :value="inputValue" | ||
| placeholder="Please enter a value" | ||
| :type="inputType === 'number' ? 'number' : 'text'" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type attribute for the <input> element is not fully implemented according to the ResolvedTextField type definition. The current implementation only distinguishes between number and text, while the type also supports email, password, tel, and url. This limits the functionality of the TextField component.
:type="inputType || 'text'"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implementation aligns with the Angular counterpart.
| userAction: { | ||
| name: action.name, | ||
| sourceComponentId: component.id, | ||
| surfaceId: surfaceId!, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a non-null assertion (!) on surfaceId can introduce a risk of runtime errors if this composable is ever used in a context where surfaceId could be null. To make the function more robust, it would be safer to add a guard clause that checks for the existence of surfaceId before proceeding to dispatch the action.
| surfaceId: surfaceId!, | |
| surfaceId: surfaceId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This follows the same implementation logic as in Angular.
| interface HintedStyles { | ||
| h1: Record<string, string>; | ||
| h2: Record<string, string>; | ||
| h3: Record<string, string>; | ||
| h4: Record<string, string>; | ||
| h5: Record<string, string>; | ||
| body: Record<string, string>; | ||
| caption: Record<string, string>; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The HintedStyles interface is missing the h6 property, but the areHintedStyles type guard on line 118 checks for it. This discrepancy can lead to incorrect type assertions. To ensure type safety and consistency, the h6 property should be added to the interface.
interface HintedStyles {
h1: Record<string, string>;
h2: Record<string, string>;
h3: Record<string, string>;
h4: Record<string, string>;
h5: Record<string, string>;
h6: Record<string, string>;
body: Record<string, string>;
caption: Record<string, string>;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This follows the same implementation logic as in Angular.
| tag: 'Layout', | ||
| surface: createSingleComponentSurface('Column', { | ||
| children: [ | ||
| createComponent('Heading', { text: { literalString: 'Heading Text' } }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The component type Heading is not defined in the DEFAULT_CATALOG, which will cause a console warning and prevent the component from rendering. To display a heading, you should use the Text component with the appropriate usageHint, such as 'h1'.
createComponent('Text', { text: { literalString: 'Heading Text' }, usageHint: 'h1' }),
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This follows the same implementation logic as in Angular.
thanks Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Description
This PR adds complete Vue 3 support to A2UI by implementing a new Vue 3 renderer and sample applications with vue3 composition api, and aligning with the existing Angular and Lit implementations. Remaining Angular samples will be added in subsequent updates.
What's Changed
I am deeply interested in the intersection of AI and front-end engineering, and I look forward to contributing to the long-term maintenance of this project