classes: accordion #1616
classes: accordion #1616
Conversation
WalkthroughThe Accordion and AccordionItem components were refactored to consolidate multiple class-related props into a single Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Accordion
participant AccordionItem
User->>Accordion: Passes { classes: { active, inactive, ... } }
Accordion->>AccordionItem: Provides classes via context
AccordionItem->>AccordionItem: Uses classes.active/inactive for styling
Possibly related PRs
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@jjagielka is attempting to deploy a commit to the Themesberg Team on Vercel. A member of the Team first needs to authorize it. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/lib/types.ts (1)
73-76: Consider improving type safety by replacinganyusage.The
ComponentVariantsgeneric type serves the refactoring well by unifying variant props with the classes object. However, theanyusage reduces type safety.Consider a more constrained approach:
-export type ComponentVariants<T extends ((...args: any) => any) & { slots: object }> = ( - VariantProps<T> & { classes?: Partial<{ [K in keyof T["slots"]]: ClassValue }> } -); +export type ComponentVariants<T extends Record<string, unknown> & { slots: object }> = ( + VariantProps<T> & { classes?: Partial<{ [K in keyof T["slots"]]: ClassValue }> } +);Alternatively, if the function constraint is necessary, consider:
-export type ComponentVariants<T extends ((...args: any) => any) & { slots: object }> = ( +export type ComponentVariants<T extends ((...args: unknown[]) => unknown) & { slots: object }> = (🧰 Tools
🪛 ESLint
[error] 74-74: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 74-74: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/lib/accordion/Accordion.svelte(2 hunks)src/lib/accordion/AccordionItem.svelte(3 hunks)src/lib/accordion/theme.ts(2 hunks)src/lib/types.ts(3 hunks)src/routes/docs/components/accordion.md(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/lib/accordion/theme.ts (2)
src/lib/types.ts (1)
ComponentVariants(74-76)src/lib/accordion/index.ts (2)
accordion(3-3)accordionitem(3-3)
src/lib/types.ts (1)
src/lib/accordion/theme.ts (2)
AccordionVariants(4-4)AccordionItemVariants(20-20)
🪛 ESLint
src/lib/types.ts
[error] 74-74: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 74-74: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Playwright and Lint
🔇 Additional comments (12)
src/lib/accordion/theme.ts (2)
1-11: LGTM! Well-structured theme refactoring for the classes approach.The conversion from individual class props to a slots-based structure is well-implemented:
- Proper imports for
ComponentVariantsandVariantProps- Logical slots structure with
base,active, andinactive- Empty strings for
active/inactiveallow flexible component-level overrides- Type aliases provide proper TypeScript integration
This foundation enables the consolidated
classesprop pattern across the accordion components.
20-20: Good addition of AccordionItemVariants type alias.This provides consistent typing for AccordionItem components, matching the pattern established for AccordionVariants.
src/routes/docs/components/accordion.md (1)
88-98: Excellent documentation update reflecting the new classes API.The documentation accurately demonstrates the transition from separate
activeClass/inactiveClassprops to the consolidatedclassesobject structure. The example clearly shows the correct usage pattern:classes={{ active: "bg-blue-100 dark:bg-gray-800 text-blue-600...", inactive: "text-gray-500 dark:text-gray-400 hover:bg-blue-100..." }}This will help users understand and adopt the new API effectively.
src/lib/accordion/Accordion.svelte (2)
5-7: Correct import and props update for the classes refactoring.The import of
AccordionPropsand props destructuring properly incorporate the newclassesprop while removing the deprecated individual class properties.
16-17: Proper context setup for the new classes structure.The context correctly extracts
activeandinactiveclasses from theclassesobject using optional chaining. This maintains the existing context pattern while supporting the new consolidated API.src/lib/accordion/AccordionItem.svelte (4)
7-9: Good addition of clsx for flexible class composition.The
clsximport and updated props destructuring properly support the newclassesobject structure while maintaining clean component interfaces.
34-34: Excellent priority chain for class resolution.The buttonClass logic properly prioritizes class sources:
- Component-level
classes?.active/inactive(highest priority)- Context-level
ctx.activeClass/inactiveClass(backward compatibility)- Theme defaults
active()/inactive()(fallback)This ensures both forward compatibility with the new API and backward compatibility with existing usage.
37-37: Clean class composition for the base element.Using
clsx(classes?.base, className)provides flexible and intuitive class merging for the base heading element.
63-63: Consistent content class application.The content class composition using
clsx(classes?.content)is applied consistently across both transition and non-transition rendering blocks, ensuring uniform styling behavior.Also applies to: 70-70
src/lib/types.ts (3)
59-59: LGTM! Import addition supports the new variant-based approach.The import of
AccordionItemVariantsandAccordionVariantscorrectly supports the accordion component refactoring to use the newclassesprop approach.
171-175: Excellent refactoring approach for accordion props consolidation.The refactoring successfully consolidates multiple class-related props into the variant-based approach:
AccordionPropsnow extendsAccordionVariantsinstead of having separateactiveClass,inactiveClass, etc.- Addition of
transitionParamsprovides enhanced animation control- This aligns perfectly with the PR objective to use a single
classesprop object
178-184: Consistent refactoring approach for AccordionItem.The
AccordionItemPropsinterface follows the same excellent refactoring pattern:
- Extends
AccordionItemVariantsfor consolidated class management- Includes
transitionParamsfor animation flexibility- Maintains consistency with the
AccordionPropsapproach
There was a problem hiding this comment.
As I mentioned it here, I think classes?.activeCtx and classes?.inactiveCtx make it clear to users that we are using them for context. What do you think?
There was a problem hiding this comment.
Ok. Not a problem to change the slots names as above, however why that's important? That's the implementation detail.
|
Yes, we should start a branch/tag called |
|
How should I push so the PR would go to the new branch? |
|
I'm wondering if it is easier to use https://github.com/themesberg/flowbite-svelte-next main branch. What do you think? Or you could do: |
📑 Description
Accordioncomponent modified to new style withclasses.ComponentVariantstype added totypes.ts.Accrodionsupplied withactiveandinactiveprops to be used inctxShould we merge it only in the new branch?
next.2.xx?Warning
Note that won't work for components with
baseonly. We can workaround that with makingslots: { base: "..."}or think of separate solution for simple components. The latter would have an advantage of NOT havingclassesproperty.Status
✅ Checks
ℹ Additional Information
Summary by CodeRabbit