-
Notifications
You must be signed in to change notification settings - Fork 39
refactor(atomic): migrate atomic-insight-result-template to Lit #6871
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
Co-authored-by: alexprudhomme <78121423+alexprudhomme@users.noreply.github.com>
Co-authored-by: alexprudhomme <78121423+alexprudhomme@users.noreply.github.com>
Co-authored-by: alexprudhomme <78121423+alexprudhomme@users.noreply.github.com>
62912b6 to
7eb9a3b
Compare
7eb9a3b to
579a11c
Compare
β¦hub.com/coveo/ui-kit into copilot/migrate-atomic-insight-template
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 the atomic-insight-result-template component from Stencil to Lit as part of the ongoing framework migration effort. The component determines the format of query results for Insight interfaces based on defined conditions.
Key Changes
- Migrated component implementation from Stencil (
.tsx) to Lit (.ts) with proper decorators and lifecycle methods - Added comprehensive unit tests using Vitest and E2E tests using Playwright
- Created Storybook documentation with stories and MDX file demonstrating usage in result lists and folded result lists
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
packages/atomic/src/components/insight/atomic-insight-result-template/atomic-insight-result-template.ts |
New Lit component implementation replacing Stencil version |
packages/atomic/src/components/insight/result-templates/atomic-insight-result-template/atomic-insight-result-template.tsx |
Removed legacy Stencil implementation |
packages/atomic/src/components/insight/atomic-insight-result-template/atomic-insight-result-template.spec.ts |
New unit tests for Lit component |
packages/atomic/src/components/insight/atomic-insight-result-template/atomic-insight-result-template.new.stories.tsx |
Storybook stories demonstrating component usage |
packages/atomic/src/components/insight/atomic-insight-result-template/atomic-insight-result-template.mdx |
Documentation with examples and template condition explanations |
packages/atomic/src/components/insight/atomic-insight-result-template/e2e/atomic-insight-result-template.e2e.ts |
E2E tests for key user scenarios |
packages/atomic/src/components/insight/atomic-insight-result-template/e2e/page-object.ts |
Page object model for E2E tests |
packages/atomic/src/components/insight/atomic-insight-result-template/e2e/fixture.ts |
Test fixture configuration |
packages/atomic/src/components/insight/index.ts |
Exports new Lit component |
packages/atomic/src/components/insight/lazy-index.ts |
Lazy loading configuration for new component |
packages/atomic/src/utils/custom-element-tags.ts |
Registers component tag name |
packages/atomic/src/components.d.ts |
Removes Stencil type definitions (now handled by Lit) |
packages/atomic-react/src/components/search/components.ts |
Adds React wrapper for search interface usage |
packages/atomic-react/src/components/commerce/components.ts |
Adds React wrapper for commerce interface usage |
π‘ Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| conditions: InsightResultTemplateCondition[] = []; | ||
|
|
||
| /** | ||
| * The field and values that define which result items the condition must be applied to. |
Copilot
AI
Jan 2, 2026
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 JSDoc comment uses inconsistent terminology. The property description says "result items" while the similar property in the search version (atomic-result-template) says just "results". For consistency across the codebase, this should say "results" instead of "result items".
| /** | ||
| * The field and values that define which result items the condition must not be applied to. | ||
| * | ||
| * For example, a template with the following attribute only applies to result items whose `filetype` is not `lithiummessage`: `must-not-match-filetype="lithiummessage"` |
Copilot
AI
Jan 2, 2026
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 JSDoc comment uses inconsistent terminology. The property description says "result items" while the similar property in the search version (atomic-result-template) says just "results". For consistency across the codebase, this should say "results" instead of "result items".
| const validParent = [ | ||
| 'atomic-insight-result-list', | ||
| 'atomic-insight-folded-result-list', | ||
| ]; | ||
| const allowEmpty = true; | ||
| this.resultTemplateController = new ResultTemplateController( | ||
| this, | ||
| validParent, |
Copilot
AI
Jan 2, 2026
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 variable naming is inconsistent with the similar component atomic-result-template. In the search version, this is named validParent (singular), but here it's validParent in the variable but contains an array. For consistency with the search component, consider renaming to validParent if keeping singular, or updating to match the exact pattern used in atomic-result-template.ts line 62.
| const validParent = [ | |
| 'atomic-insight-result-list', | |
| 'atomic-insight-folded-result-list', | |
| ]; | |
| const allowEmpty = true; | |
| this.resultTemplateController = new ResultTemplateController( | |
| this, | |
| validParent, | |
| const validParents = [ | |
| 'atomic-insight-result-list', | |
| 'atomic-insight-folded-result-list', | |
| ]; | |
| const allowEmpty = true; | |
| this.resultTemplateController = new ResultTemplateController( | |
| this, | |
| validParents, |
| const template = | ||
| this.resultTemplateController?.getTemplate(this.conditions) || null; | ||
| return template; | ||
| } |
Copilot
AI
Jan 2, 2026
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 unnecessary ternary expression and optional chaining can be simplified. Since getTemplate already returns null when the controller returns null/undefined, the expression can be simplified to: return this.resultTemplateController.getTemplate(this.conditions);
| const template = | |
| this.resultTemplateController?.getTemplate(this.conditions) || null; | |
| return template; | |
| } | |
| return this.resultTemplateController.getTemplate(this.conditions); | |
| } |
| it('should call getTemplate on the controller', async () => { | ||
| const titleConditions = (result: InsightResult) => | ||
| result.title === 'Coveo'; | ||
| const fakeTemplate = { | ||
| conditions: [], | ||
| content: document.createDocumentFragment(), | ||
| priority: 1, | ||
| }; | ||
| const spy = vi | ||
| .spyOn(ResultTemplateController.prototype, 'getTemplate') | ||
| .mockResolvedValue(fakeTemplate); | ||
| const element = await setupElement({conditions: [titleConditions]}); | ||
| const result = await element.getTemplate(); | ||
|
|
||
| expect(spy).toHaveBeenCalledWith([titleConditions]); | ||
| expect(result).toBe(fakeTemplate); | ||
| spy.mockRestore(); |
Copilot
AI
Jan 2, 2026
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 spy is not used after being created. The test creates a spy on ResultTemplateController.prototype.getTemplate and mocks it to return a fake template, then verifies the spy was called and the result matches. However, the spy should be cleaned up in an afterEach hook or at the end of the test to avoid affecting other tests. The mockRestore() call at line 109 handles this, but consider using a nested describe block with beforeEach/afterEach for better test isolation and consistency with testing guidelines.
| it('should call getTemplate on the controller', async () => { | |
| const titleConditions = (result: InsightResult) => | |
| result.title === 'Coveo'; | |
| const fakeTemplate = { | |
| conditions: [], | |
| content: document.createDocumentFragment(), | |
| priority: 1, | |
| }; | |
| const spy = vi | |
| .spyOn(ResultTemplateController.prototype, 'getTemplate') | |
| .mockResolvedValue(fakeTemplate); | |
| const element = await setupElement({conditions: [titleConditions]}); | |
| const result = await element.getTemplate(); | |
| expect(spy).toHaveBeenCalledWith([titleConditions]); | |
| expect(result).toBe(fakeTemplate); | |
| spy.mockRestore(); | |
| const fakeTemplate = { | |
| conditions: [], | |
| content: document.createDocumentFragment(), | |
| priority: 1, | |
| }; | |
| let controllerGetTemplateSpy: ReturnType<typeof vi.spyOn>; | |
| beforeEach(() => { | |
| controllerGetTemplateSpy = vi | |
| .spyOn(ResultTemplateController.prototype, 'getTemplate') | |
| .mockResolvedValue(fakeTemplate); | |
| }); | |
| afterEach(() => { | |
| controllerGetTemplateSpy.mockRestore(); | |
| }); | |
| it('should call getTemplate on the controller', async () => { | |
| const titleConditions = (result: InsightResult) => | |
| result.title === 'Coveo'; | |
| const element = await setupElement({conditions: [titleConditions]}); | |
| const result = await element.getTemplate(); | |
| expect(controllerGetTemplateSpy).toHaveBeenCalledWith([ | |
| titleConditions, | |
| ]); | |
| expect(result).toBe(fakeTemplate); |
| > | ||
|
|
||
| This component defines the UI display of your search results within an Insight interface. | ||
| A `template` element must be the child of an `atomic-insight-result-template`. Furthermore, an `atomic-insight-result-list` or `atomic-insight-folded-result-list` must be the parent of each `atomic-insight-result-template`. |
Copilot
AI
Jan 2, 2026
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 documentation uses inconsistent terminology. Throughout this file, "result items" is used (lines 16, 42, 50, 54, etc.), but the search component version uses "results" for consistency. The MDX documentation should use "results" instead of "result items" to maintain consistency with atomic-result-template documentation.
β Checklist
.mdxfileindex.tsandlazy-index.tsfiles.https://coveord.atlassian.net/browse/KIT-5345