-
Notifications
You must be signed in to change notification settings - Fork 39
refactor(atomic): migrate atomic-insight-user-actions-toggle to Lit #6879
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>
β¦tions-toggle Co-authored-by: alexprudhomme <78121423+alexprudhomme@users.noreply.github.com>
Co-authored-by: alexprudhomme <78121423+alexprudhomme@users.noreply.github.com>
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 successfully migrates the atomic-insight-user-actions-toggle component from Stencil to Lit. The component displays a button that opens a modal containing user actions timeline, designed for Insight Panel interfaces.
Key Changes
- Migrated component from Stencil (
.tsx) to Lit (.ts) using modern decorators and patterns - Replaced Stencil's
IconButtonwith the Lit functionalrenderIconButtoncomponent - Added comprehensive unit tests using Vitest and E2E tests using Playwright
- Created Storybook documentation (
.mdxfile) and stories
Reviewed changes
Copilot reviewed 10 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
atomic-insight-user-actions-toggle.ts |
New Lit implementation with proper decorators, bindings, and error guards |
atomic-insight-user-actions-toggle.tsx |
Removed legacy Stencil implementation |
atomic-insight-user-actions-toggle.pcss |
Removed (no longer needed with shadow DOM component) |
atomic-insight-user-actions-toggle.spec.ts |
New comprehensive unit tests covering initialization, rendering, and modal interaction |
atomic-insight-user-actions-toggle.new.stories.tsx |
Storybook stories with MSW API mocking |
atomic-insight-user-actions-toggle.mdx |
Component documentation for Storybook |
e2e/atomic-insight-user-actions-toggle.e2e.ts |
Playwright E2E tests for button visibility and modal interaction |
e2e/page-object.ts |
Page object pattern for E2E test interactions |
e2e/fixture.ts |
E2E test fixtures and setup |
user-actions-controller.ts |
New test helper fixture for mocking UserActions controller |
lazy-index.ts |
Added lazy loading entry for the migrated component |
index.ts |
Added component export |
custom-element-tags.ts |
Registered new custom element tag |
components.d.ts |
Auto-generated type definitions updated (Stencil types removed) |
π‘ Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public userActions!: InsightUserActions; | ||
|
|
||
| @bindStateToController('userActions') | ||
| @state() | ||
| public userActionsState!: InsightUserActionsState; | ||
|
|
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 userActionsState property should be declared as private. According to the class field declaration order guidelines, controller-bound state fields should be grouped with other state fields, but public state that's bound to controllers should typically be private unless explicitly needed in the public API. Since this state is only used internally to react to controller state changes and isn't documented as part of the component's public API, it should be private.
| public userActions!: InsightUserActions; | |
| @bindStateToController('userActions') | |
| @state() | |
| public userActionsState!: InsightUserActionsState; | |
| @bindStateToController('userActions') | |
| @state() | |
| private userActionsState!: InsightUserActionsState; | |
| public userActions!: InsightUserActions; |
| @state() | ||
| error!: Error; | ||
|
|
||
| public userActions!: InsightUserActions; |
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 userActions property should be declared as private. This controller is only used internally by the component and is not documented as part of the public API. According to the component structure guidelines, fields that are not part of the public API should be private to avoid exposing implementation details.
| public userActions!: InsightUserActions; | |
| private userActions!: InsightUserActions; |
| private buttonRef?: HTMLButtonElement; | ||
| private modalRef?: HTMLAtomicInsightUserActionsModalElement; |
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 property declaration order doesn't follow the established guidelines. According to the class field declaration order, non-decorated fields (like buttonRef and modalRef) should come after all @state decorated fields. Move these two properties to appear after the userActionsState declaration.
β Checklist
.mdxfileindex.tsandlazy-index.tsfiles.https://coveord.atlassian.net/browse/KIT-5362