-
Notifications
You must be signed in to change notification settings - Fork 39
refactor(atomic): migrate atomic-timeframe-facet from Stencil to Lit #6757
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 <[email protected]>
Co-authored-by: fbeaudoincoveo <[email protected]>
…W API mocking Co-authored-by: fbeaudoincoveo <[email protected]>
…tests for atomic-timeframe-facet Co-authored-by: fbeaudoincoveo <[email protected]>
| tabsExcluded: this.tabsExcluded, | ||
| dependsOn: this.dependsOn, | ||
| }), | ||
| new Schema({ |
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.
@copilot do like in atomic-color-facet: declare the schema as a static private readonly member of the class so that it's only declared once even if there are multiple facet instances in the same page.
Also, I wouldn't include tabsInclude, tabsExcluded and dependsOn in the prop validation. We should validate the following props:
- injectionDepth
- headingLevel
- min
- max
- sortCriteria
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.
Updated in commit 0e2d926. The schema is now declared as a static private readonly member, and prop validation only includes injectionDepth, headingLevel, min, max, and sortCriteria.
| }; | ||
| }; | ||
|
|
||
| describe('#render', () => { |
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.
| describe('#render', () => { | |
| describe('when rendering (#render)', () => { |
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.
Updated in commit 0e2d926.
| }); | ||
| }); | ||
|
|
||
| describe('#props', () => { |
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.
| describe('#props', () => { | |
| describe('props', () => { |
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.
Updated in commit 0e2d926.
| }); | ||
| }); | ||
|
|
||
| describe('#lifecycle', () => { |
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.
@copilot remove describe wrapper.
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.
Updated in commit 0e2d926. The describe wrapper has been removed.
| }); | ||
|
|
||
| describe('#lifecycle', () => { | ||
| describe('disconnectedCallback', () => { |
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.
| describe('disconnectedCallback', () => { | |
| describe('when removed from the DOM (#disconnectedCallback)', () => { |
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.
Updated in commit 0e2d926.
| }); | ||
| }); | ||
|
|
||
| describe('#interactions', () => { |
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.
@copilot remove describe wrapper.
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.
Updated in commit 0e2d926. The describe wrapper has been removed and tests merged into parent describe block.
| }); | ||
| }); | ||
|
|
||
| describe('#controllers', () => { |
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.
@copilot remove describe wrapper
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.
Updated in commit 0e2d926. The describe wrapper has been removed.
packages/atomic/src/components/search/atomic-timeframe-facet/atomic-timeframe-facet.ts
Show resolved
Hide resolved
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.
@copilot this file must be deleted, not renamed.
also, the atomic-timeframe-facet.pcss file must be deleted as well.
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.
Updated in commit 0e2d926. Both stencil-atomic-timeframe-facet.tsx and atomic-timeframe-facet.pcss have been deleted.
| @state() bindings!: Bindings; | ||
| @state() error!: Error; | ||
|
|
||
| @bindStateToController('facetForDateRange') |
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.
| @bindStateToController('facetForDateRange') | |
| // biome-ignore lint/suspicious/noExplicitAny: bindStateToController requires non-optional Controller type, but these controllers are conditionally created based on component configuration | |
| @bindStateToController('facetForDateRange' as any) |
similar comment for the facetForDatePicker and filter bindStateToController decorators.
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.
Updated in commit 0e2d926. Added biome-ignore comments for all three conditional bindStateToController decorators (facetForDateRange, facetForDatePicker, and filter).
…ation - Move atomic-timeframe-facet directory from /facets to /search level - Delete stencil-atomic-timeframe-facet.tsx and atomic-timeframe-facet.pcss files - Update prop validation to use static readonly schema and only validate specific props (injectionDepth, headingLevel, min, max, sortCriteria) - Add biome-ignore comments for conditional bindStateToController decorators - Restructure test describe blocks: remove unnecessary wrappers, update naming conventions, combine nested describe blocks Co-authored-by: fbeaudoincoveo <[email protected]>
Migration complete with all feedback addressed:
Component Changes:
/facets/atomic-timeframe-facet/to/search/atomic-timeframe-facet/stencil-atomic-timeframe-facet.tsxandatomic-timeframe-facet.pcssTest Changes:
'when rendering (#render)'and'props''when removed from the DOM (#disconnectedCallback)'Export Changes:
search/index.tsOriginal prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.