-
Notifications
You must be signed in to change notification settings - Fork 39
refactor(atomic): Migrate SmartSnippet functional components #6602
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
Migrate 5 SmartSnippet functional components from Stencil to Lit: - SmartSnippetWrapper - SmartSnippetQuestion - SmartSnippetTruncatedAnswer - SmartSnippetFooter - SmartSnippetFeedbackBanner Changes: - Created new Lit versions of all functional components - Renamed original Stencil files with `stencil-` prefix - Updated imports in components using these functions - Added comprehensive unit tests for all migrated components - All tests passing and linting clean Co-authored-by: alexprudhomme <[email protected]>
...components/common/smart-snippets/atomic-smart-snippet/smart-snippet-functional-components.ts
Outdated
Show resolved
Hide resolved
|
@copilot retry this please ! |
Co-authored-by: alexprudhomme <[email protected]>
This reverts commit 56fa228.
Co-Authored-By: alexprudhomme <[email protected]>
Co-Authored-By: alexprudhomme <[email protected]>
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.
Pull Request Overview
This PR migrates SmartSnippet functional components from Stencil to Lit, establishing a clear migration path by creating new Lit implementations while preserving the legacy Stencil versions with deprecation markers.
Key Changes:
- Created Lit-based functional components following established patterns (render* naming, FunctionalComponent types)
- Renamed legacy Stencil components with
stencil-prefix and added deprecation markers - Added comprehensive test coverage for all new Lit components
- Updated imports in consuming Stencil components to reference the renamed legacy versions
Reviewed Changes
Copilot reviewed 12 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
atomic-smart-snippet.tsx |
Updated imports to use stencil- prefixed legacy components |
atomic-insight-smart-snippet.tsx |
Updated imports to use stencil- prefixed legacy components |
stencil-smart-snippet-feedback-banner.tsx |
Renamed legacy Stencil feedback banner with deprecation marker |
smart-snippet-feedback-banner.ts |
New Lit implementation of feedback banner functional component |
smart-snippet-feedback-banner.spec.ts |
Comprehensive test coverage for Lit feedback banner (251 test cases) |
stencil-smart-snippet-common.tsx |
Renamed legacy Stencil common components with deprecation markers |
snippet-wrapper.ts |
New Lit wrapper component for smart snippets |
snippet-wrapper.spec.ts |
Test coverage for wrapper component |
snippet-question.ts |
New Lit question heading component |
snippet-question.spec.ts |
Test coverage for question component |
snippet-truncated-answer.ts |
New Lit truncated answer component |
snippet-truncated-answer.spec.ts |
Test coverage for truncated answer component |
snippet-footer.ts |
New Lit footer component |
snippet-footer.spec.ts |
Test coverage for footer component |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }), | ||
| } as unknown as i18n; | ||
| }); | ||
|
|
Copilot
AI
Nov 20, 2025
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 container element is appended to document.body in beforeEach but never removed in an afterEach. This could cause memory leaks and test pollution. Add cleanup:
afterEach(() => {
document.body.removeChild(container);
});| afterEach(() => { | |
| if (container && container.parentNode === document.body) { | |
| document.body.removeChild(container); | |
| } | |
| }); |
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.
Fair catch. @alexprudhomme if you don't use the render fixture, you have to own the clean up ;)
| class=${ | ||
| props.liked | ||
| ? 'flex items-center gap-x-1.5 text-success' | ||
| : 'flex items-center gap-x-1.5 cursor-pointer hover:underline' | ||
| } |
Copilot
AI
Nov 20, 2025
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.
[nitpick] The common classes 'flex items-center gap-x-1.5' are duplicated in both branches of the ternary. Consider using the classMap directive or separating common classes to improve maintainability:
class="flex items-center gap-x-1.5 ${props.liked ? 'text-success' : 'cursor-pointer hover:underline'}"Or using classMap:
class=${classMap({
'flex': true,
'items-center': true,
'gap-x-1.5': true,
'text-success': props.liked,
'cursor-pointer': !props.liked,
'hover:underline': !props.liked,
})}The same applies to the dislike button classes on lines 58-62.
jmazzah-coveo
left a comment
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.
why are there new components with this migration? i don't see them used anywhere. like snippet-footer, snippet-question, etc? will we need them in the future?
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.