-
Notifications
You must be signed in to change notification settings - Fork 39
refactor(atomic): migrate tab-button to Lit functional component #6647
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>
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 atomic-tab-button from a Stencil class-based component to a Lit functional component, following the pattern used in other recent migrations. The original Stencil component is renamed to stencil-tab-button.tsx for backward compatibility, while a new renderTabButton functional component is created using Lit directives and Tailwind CSS.
Key Changes:
- Creates functional component with TypeScript types and proper Tailwind CSS styling
- Preserves all functionality including active/inactive states, ARIA attributes, shadow parts, and click handlers
- Implements comprehensive unit tests using
renderFunctionFixturehelper
Issues Identified:
- Incorrect functional component type (accepts children but doesn't use them)
- Accessibility concerns with ARIA attributes and roles
- Test coverage gaps related to implementation issues
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| tab-button.ts | New Lit functional component implementation with Tailwind CSS styling |
| tab-button.spec.ts | Comprehensive unit tests for the functional component |
| stencil-tab-button.tsx | Original Stencil component renamed for backward compatibility |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import {html} from 'lit'; | ||
| import {renderButton} from '@/src/components/common/button'; | ||
| import {multiClassMap, tw} from '@/src/directives/multi-class-map'; | ||
| import type {FunctionalComponentWithChildren} from '@/src/utils/functional-component-utils'; |
Copilot
AI
Nov 27, 2025
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.
Missing import for ifDefined directive. The suggestion in the previous comments to use ifDefined(props.active ? 'true' : undefined) for the aria-current attribute requires importing this directive from lit/directives/if-defined.js.
Add to imports:
import {ifDefined} from 'lit/directives/if-defined.js';This follows the pattern used in button.ts and other functional components.
|
My overall assessment: we should fix the component type right now. It doesn't deal with children, so it should not be a FunctionalComponentWithChildren. We should also do the ifDefined thing with aria-selected and use the correct optional attribute pattern. The rest of the accessibility-related comments seem correct after verification, but I would personally defer and do them in a subsequent tab accessibility improvement PR. |
fbeaudoincoveo
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.
(my conclusion is "request changes")
Migration Plan: atomic-tab-button to Lit Functional Component
Migration Type: Functional Component Migration
Analysis
atomic-tab-buttonis currently a Stencil component (class-based with@Componentdecorator)Button→renderButtonButtonfunctional component internallyatomic-tab-manager(still Stencil)Migration Steps
stencil-tab-button.tsxtab-button.tswithrenderTabButtontab-button.spec.tsrenderFunctionFixturehelper (PR feedback)Implementation Details
renderTabButtonfunctional component with proper TypeScript typestwandmultiClassMapfor Tailwind CSS classesrenderFunctionFixturehelper (standard pattern for functional component tests)Changes Made
Commit 1: Initial migration from Stencil to Lit functional component
Commit 2: Updated tests to use
renderFunctionFixturehelper per PR feedbackOriginal prompt
KIT-5024
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.