-
Notifications
You must be signed in to change notification settings - Fork 39
chore(atomic): Prepare atomic-quickview-sidebar to migration to Lit #6636
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
Conversation
9174484 to
4e72f84
Compare
c8be29e to
3ecff48
Compare
3ecff48 to
1abb405
Compare
alexprudhomme
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.
Lgtm 👍
df68b56 to
99a0760
Compare
1abb405 to
9332263
Compare
38f7bd8 to
055258b
Compare
...result-template-components/atomic-quickview-sidebar/stencil-atomic-quickview-sidebar.spec.ts
Dismissed
Show dismissed
Hide dismissed
...ult-template-components/atomic-quickview-sidebar/stencil-highlight-keywords-checkbox.spec.ts
Dismissed
Show dismissed
Hide dismissed
...s/search/result-template-components/atomic-quickview-sidebar/stencil-minimize-button.spec.ts
Fixed
Show fixed
Hide fixed
...mponents/search/result-template-components/atomic-quickview-sidebar/stencil-keywords.spec.ts
Dismissed
Show dismissed
Hide dismissed
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 prepares the atomic-quickview-sidebar component for migration to Lit by refactoring the Stencil functional component structure. The changes maintain existing functionality while adhering to patterns that will support the upcoming Lit migration.
Key changes:
- Enhanced
renderStencilVNodetest utility to support arrays, event listeners,htmlForattribute mapping, and boolean attributes - Split monolithic component into focused, testable functional components with
stencil-prefixes - Replaced
Fragmentwith array returns for Vitest compatibility - Generated comprehensive unit tests for all functional components
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
vitest-utils/testing-helpers/stencil-vnode-renderer.ts |
Added support for array rendering, event listeners (onClick, etc.), htmlFor attribute conversion, and boolean attributes to enable testing of split functional components |
atomic-quickview-sidebar/stencil-minimize-button.tsx |
Extracted minimize button logic into standalone functional component |
atomic-quickview-sidebar/stencil-minimize-button.spec.ts |
Added unit tests covering button rendering, event handling, and conditional badge display |
atomic-quickview-sidebar/stencil-keywords.tsx |
Extracted keywords list rendering; replaced Fragment with array return pattern |
atomic-quickview-sidebar/stencil-keywords.spec.ts |
Added tests for keyword rendering, navigation callbacks, highlight toggling, and disabled states |
atomic-quickview-sidebar/stencil-highlight-keywords-checkbox.tsx |
Extracted checkbox component; uses array return for conditional label rendering |
atomic-quickview-sidebar/stencil-highlight-keywords-checkbox.spec.ts |
Added tests for checkbox state management and conditional label visibility |
atomic-quickview-sidebar/stencil-atomic-quickview-sidebar.tsx |
Main component now orchestrates extracted child components; marked as deprecated for Stencil-only use |
atomic-quickview-sidebar/stencil-atomic-quickview-sidebar.spec.ts |
Added integration tests for component composition and state propagation |
atomic-quickview-modal/atomic-quickview-modal.tsx |
Updated import path to reference renamed stencil-atomic-quickview-sidebar file |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...result-template-components/atomic-quickview-sidebar/stencil-atomic-quickview-sidebar.spec.ts
Outdated
Show resolved
Hide resolved
...result-template-components/atomic-quickview-sidebar/stencil-atomic-quickview-sidebar.spec.ts
Outdated
Show resolved
Hide resolved
...s/search/result-template-components/atomic-quickview-sidebar/stencil-minimize-button.spec.ts
Outdated
Show resolved
Hide resolved
packages/atomic/vitest-utils/testing-helpers/stencil-vnode-renderer.ts
Outdated
Show resolved
Hide resolved
...result-template-components/atomic-quickview-sidebar/stencil-atomic-quickview-sidebar.spec.ts
Show resolved
Hide resolved
...ult-template-components/atomic-quickview-sidebar/stencil-highlight-keywords-checkbox.spec.ts
Show resolved
Hide resolved
...mponents/search/result-template-components/atomic-quickview-sidebar/stencil-keywords.spec.ts
Show resolved
Hide resolved
...s/search/result-template-components/atomic-quickview-sidebar/stencil-minimize-button.spec.ts
Show resolved
Hide resolved
055258b to
edec312
Compare
Work for #6389
https://coveord.atlassian.net/browse/KIT-4982
This is a PR in preparation to the migration to Lit: #6662
I decided to do it separately because the PR was already big and I wanted to make sure my changes were still passing all CI.
stencil-<Fragment>.renderStencilVNode:htmlFor, which resolves to theforattribute.