-
Notifications
You must be signed in to change notification settings - Fork 39
refactor(atomic): migrate atomic-insight-facet from Stencil to Lit #6972
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: fbeaudoincoveo <23503066+fbeaudoincoveo@users.noreply.github.com>
Co-authored-by: fbeaudoincoveo <23503066+fbeaudoincoveo@users.noreply.github.com>
|
@copilot There are several ts errors in atomic-insight-facet.ts. Also atomic-insight-facet.tsx has not been deleted, nor has atomic-insight-facet.pcss. This should be done. For the pcss file, double check to ensure that the static styles property in the migrated component is fully equivalent. Once the old files have been deleted, and the necessary adjustments have been made to make the build pass, continue to the next step (creating Storybook stories). |
β¦tomic-insight-facet Co-authored-by: fbeaudoincoveo <23503066+fbeaudoincoveo@users.noreply.github.com>
Fixed in commit 37b563e: TypeScript errors fixed:
Old files deleted:
The build now passes successfully. Proceeding to create Storybook stories next. |
β¦ mocking Co-authored-by: fbeaudoincoveo <23503066+fbeaudoincoveo@users.noreply.github.com>
| * - `allValues`: A result will match if all corresponding facet values are selected. | ||
| */ | ||
| @property({type: String, reflect: true, attribute: 'results-must-match'}) | ||
| resultsMustMatch: FacetResultsMustMatch = 'atLeastOneValue'; |
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.
Note: this prop is entirely unused at the moment, but it was in the original Stencil component so I didn't remove it.
However, given the fact that insight components are more or less "internal", we could consider removing iat entirely.
β Checklist
.mdxfileindex.tsandlazy-index.tsfiles.https://coveord.atlassian.net/browse/KIT-4853
Description
Migrates
atomic-insight-facetfrom Stencil to Lit with complete test coverage, Storybook documentation, and accessibility compliance.Changes
Component Migration
@customElement,@bindings(),@withTailwindStylesdecoratorswillUpdate,render,disconnectedCallback)htmltemplate literalswhen()directive for conditional rendering instead ofFacetGuard@/src/path aliases (no relative../imports except parent interface)HTMLElementTagNameMapdeclaration for type safety@internaltag from JSDoc to make component publicly documented and Storybook-compatibleFacetResultsMustMatchimport from@coveo/headless/insightto@coveo/headlessFocusTargetControllerconstructor calls to include requirednameparameterAriaLiveRegionControllerusage to use.messageproperty instead of.announce()methodheaderRefandsetRefatomic-insight-facet.tsx,atomic-insight-facet.pcss)Unit Tests
facet-controller.ts,facet-conditions-manager.ts)Storybook Stories
atomic-insight-facet.new.stories.tsxwith MSW API mockingMockInsightApifor proper API response mockingwrapInInsightInterfacefor proper Insight interface context@internaltag to enable Storybook documentationE2E Tests
e2e/directory:fixture.ts- Test fixture with InsightFacet page objectpage-object.ts- Page object with locators for facet elements (expand buttons, facet values, show more/less, clear button)atomic-insight-facet.e2e.ts- E2E tests covering:MDX Documentation
atomic-insight-facet.mdxwith comprehensive documentation:Cypress Tests
Original prompt
π¬ We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.