-
Notifications
You must be signed in to change notification settings - Fork 39
refactor(atomic): migrate atomic-tab-button from Stencil to Lit #6711
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
- Create new atomic-tab-button component as a Lit custom element in its own folder - Delete incorrect functional component migration (tab-button.ts) - Delete original Stencil component (stencil-tab-button.tsx) and styles (tab-button.pcss) - Add comprehensive unit tests for the new component - Update exports in index.ts and lazy-index.ts - Add import to atomic-tab-manager.ts 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 migrates the atomic-tab-button component from Stencil to Lit as part of the broader Stencil-to-Lit migration effort. The component is marked as internal, so e2e tests, Storybook stories, and MDX documentation are not required per the PR description.
Key Changes:
- Migrated
atomic-tab-buttonfrom Stencil to Lit with Tailwind CSS styling - Updated unit tests to use Vitest instead of the previous testing framework
- Updated
atomic-tab-managerto use the new Lit component and corrected role from "list" to "tablist"
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
packages/atomic/src/components/common/atomic-tab-button/atomic-tab-button.ts |
New Lit implementation of tab button component with shadow DOM and Tailwind styles |
packages/atomic/src/components/common/atomic-tab-button/atomic-tab-button.spec.ts |
Migrated unit tests to Vitest with updated test structure |
packages/atomic/src/components/search/atomic-tab-manager/atomic-tab-manager.ts |
Updated to import and use new Lit tab button, changed container role from "list" to "tablist" |
packages/atomic/src/components/search/atomic-tab-manager/e2e/page-object.ts |
Updated locator from label-based to role-based for better semantic querying |
packages/atomic/src/components/common/index.ts |
Exported new AtomicTabButton component |
packages/atomic/src/components/common/lazy-index.ts |
Added lazy loading entry for atomic-tab-button |
packages/atomic/src/utils/custom-element-tags.ts |
Registered 'atomic-tab-button' custom element tag |
packages/atomic/src/components/common/tabs/stencil-tab-button.tsx |
Removed old Stencil implementation |
packages/atomic/src/components/common/tabs/tab-button.ts |
Removed old functional component implementation |
packages/atomic/src/components/common/tabs/tab-button.pcss |
Removed old PostCSS styles |
packages/atomic/src/components.d.ts |
Removed Stencil-generated type definitions for atomic-tab-button |
Comments suppressed due to low confidence (2)
packages/atomic/src/components/common/atomic-tab-button/atomic-tab-button.spec.ts:51
- The test is checking for
role="listitem"but the implementation usesrole="tab". This test should be updated to verifyrole="tab"instead. The container queryquerySelector('[role="listitem"]')on line 25 will return null, causing test failures.
packages/atomic/src/components/common/atomic-tab-button/atomic-tab-button.spec.ts:25 - The
getContainer()function queries for'[role="listitem"]'but the implementation rendersrole="tab". This selector should be'[role="tab"]'to match the actual implementation.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/atomic/src/components/common/atomic-tab-button/atomic-tab-button.ts
Show resolved
Hide resolved
packages/atomic/src/components/common/atomic-tab-button/atomic-tab-button.ts
Show resolved
Hide resolved
packages/atomic/src/components/common/atomic-tab-button/atomic-tab-button.ts
Show resolved
Hide resolved
3bb1453 to
5348147
Compare
5348147 to
1535b44
Compare
1535b44 to
6774f52
Compare
No e2e, stories or mdx needed as this is an internal component.
✅ Checklist
.mdxfileindex.tsandlazy-index.tsfiles.https://coveord.atlassian.net/browse/KIT-5024
close #6377