-
Notifications
You must be signed in to change notification settings - Fork 39
feat(atomic): migrate atomic-facet-number-input to Lit #6744
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
feat(atomic): migrate atomic-facet-number-input to Lit #6744
Conversation
…tep 0) - Add visual regression tests to numeric facet E2E - Create page object helpers for screenshots - Add WithInputInteger story for testing - Generate baseline snapshot from Stencil component - Snapshot will be used to validate Lit migration Co-authored-by: jmazzah-coveo <[email protected]>
- Create new atomic-facet-number-input.ts (Lit version) - Rename original to stencil-facet-number-input.tsx - Use @/* path aliases for all imports - Implement InitializableComponent pattern - Add CSS styles inline with css tag - Add event handling for form submission - Preserve all Stencil functionality in Lit Co-authored-by: jmazzah-coveo <[email protected]>
- Create comprehensive Vitest unit tests - Test props, rendering, form submission, validation - Test integer vs decimal step behavior - Test refs and event handling - Follow commerce variant test patterns - All 17 tests passing Co-authored-by: jmazzah-coveo <[email protected]>
- Add functional E2E tests for number input - Test rendering, filling values, applying range - Test breadcrumb integration - Test clear filter functionality - Use proper test synchronization with await expect() - Document page object usage (parent is Stencil) Co-authored-by: jmazzah-coveo <[email protected]>
- Add input validation tests to Playwright E2E - Test empty inputs, missing inputs, and invalid ranges - Categorize Cypress tests: * Display/interaction tests: Duplicate (covered in Playwright) * Validation tests: Migrated to Playwright * Analytics/URL tests: Out of scope (parent responsibility) - Cypress tests can remain for now (parent still Stencil) Co-authored-by: jmazzah-coveo <[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 the internal atomic-facet-number-input component from Stencil to Lit, converting it to a standalone Lit component while removing the legacy Stencil implementation. The component is used by both search and insight numeric facets to provide number input functionality.
Key Changes:
- Migrates component implementation from Stencil to Lit with light DOM rendering
- Converts PostCSS styles to inline Lit
csstags - Adds comprehensive unit tests (17 tests) and E2E validation tests
- Registers component in lazy loading system and exports
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
packages/atomic/src/components/common/atomic-facet-number-input/atomic-facet-number-input.ts |
New Lit implementation with decorators, light DOM mixin, and inline styles |
packages/atomic/src/components/common/atomic-facet-number-input/atomic-facet-number-input.spec.ts |
New unit tests covering props, rendering, form submission, and validation |
packages/atomic/src/components/common/facets/facet-number-input/facet-number-input.tsx |
Removed original Stencil implementation |
packages/atomic/src/components/common/facets/facet-number-input/atomic-facet-number-input.pcss |
Removed PostCSS styles (converted to inline CSS) |
packages/atomic/src/components/search/facets/atomic-numeric-facet/e2e/atomic-numeric-facet.e2e.ts |
Added validation tests for empty inputs and min/max validation |
packages/atomic/src/components/search/facets/atomic-numeric-facet/atomic-numeric-facet.new.stories.tsx |
Added story for integer input testing |
packages/atomic/src/utils/custom-element-tags.ts |
Registered component tag in custom elements set |
packages/atomic/src/components/common/lazy-index.ts |
Added lazy loading entry for the component |
packages/atomic/src/components/common/index.ts |
Added export for the new component |
packages/atomic/src/components.d.ts |
Removed Stencil type definitions (auto-generated) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/atomic/src/components/common/atomic-facet-number-input/atomic-facet-number-input.ts
Outdated
Show resolved
Hide resolved
packages/atomic/src/components/common/atomic-facet-number-input/atomic-facet-number-input.ts
Outdated
Show resolved
Hide resolved
packages/atomic/src/components/common/atomic-facet-number-input/atomic-facet-number-input.ts
Outdated
Show resolved
Hide resolved
packages/atomic/src/components/common/atomic-facet-number-input/atomic-facet-number-input.ts
Outdated
Show resolved
Hide resolved
packages/atomic/src/components/common/atomic-facet-number-input/atomic-facet-number-input.ts
Outdated
Show resolved
Hide resolved
packages/atomic/src/components/common/atomic-facet-number-input/atomic-facet-number-input.ts
Show resolved
Hide resolved
packages/atomic/src/components/common/atomic-facet-number-input/atomic-facet-number-input.ts
Show resolved
Hide resolved
packages/atomic/src/components/common/atomic-facet-number-input/atomic-facet-number-input.ts
Outdated
Show resolved
Hide resolved
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
Migrates the internal
atomic-facet-number-inputcomponent from Stencil to Lit while maintaining backward compatibility for Stencil parent components.Changes
Component Migration
atomic-facet-number-input.tswith Lit implementationstencil-facet-number-input.tsxfor continued use by Stencil parentsInitializableComponentpattern with@bindings()decoratorcsstag@/*path aliases throughoutTesting
Backward Compatibility
Both versions coexist until parent components (
atomic-numeric-facet,atomic-insight-numeric-facet) are migrated from Stencil. Parents continue usingstencil-facet-number-input.tsxwith no breaking changes.Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.