Conversation
|
@jjagielka is attempting to deploy a commit to the Themesberg Team on Vercel. A member of the Team first needs to authorize it. |
|
Caution Review failedThe pull request is closed. WalkthroughThis update centralizes and modernizes component styling by introducing unified Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Component
participant ThemeUtils
User->>Component: Passes props (including classes or deprecated props)
Component->>ThemeUtils: Calls themeDeprecated if deprecated props used
ThemeUtils-->>Component: Logs deprecation warning (if needed)
Component->>Component: Derives styling from classes or deprecated props
Component->>Component: Applies styling to elements
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
src/lib/types.ts (6)
376-376: Consider replacing empty interface with type alias.The ESLint warning is valid - this empty interface is equivalent to its supertype. Consider replacing it with a type alias for better clarity:
-export interface CheckIconProps extends SVGAttributes<SVGSVGElement> { } +export type CheckIconProps = SVGAttributes<SVGSVGElement>;
594-594: Consider replacing empty interface with type alias.-export interface DropdownDividerProps extends HTMLAttributes<HTMLDivElement> { } +export type DropdownDividerProps = HTMLAttributes<HTMLDivElement>;
1092-1092: Consider replacing empty interface with type alias.-export interface NavBrandProps extends HTMLAnchorAttributes { } +export type NavBrandProps = HTMLAnchorAttributes;
1128-1128: Consider replacing empty interface with type alias.-export interface ToolbarGroupProps extends ToolbarGroupVariants, HTMLAttributes<HTMLDivElement> { } +export type ToolbarGroupProps = ToolbarGroupVariants & HTMLAttributes<HTMLDivElement>;
1437-1437: Consider replacing empty interface with type alias.-export interface TestimonialPlaceholderProps extends HTMLAttributes<HTMLDivElement> { } +export type TestimonialPlaceholderProps = HTMLAttributes<HTMLDivElement>;
1447-1447: Consider replacing empty interface with type alias.-export interface WidgetPlaceholderProps extends HTMLAttributes<HTMLDivElement> { } +export type WidgetPlaceholderProps = HTMLAttributes<HTMLDivElement>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/lib/accordion/AccordionItem.svelte(3 hunks)src/lib/accordion/theme.ts(1 hunks)src/lib/avatar/theme.ts(1 hunks)src/lib/badge/Badge.svelte(2 hunks)src/lib/badge/theme.ts(1 hunks)src/lib/banner/Banner.svelte(2 hunks)src/lib/banner/theme.ts(1 hunks)src/lib/theme/themeUtils.ts(1 hunks)src/lib/types.ts(13 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: superstes
PR: themesberg/flowbite-svelte#0
File: :0-0
Timestamp: 2025-03-20T11:52:09.106Z
Learning: In the Flowbite Svelte library's form components, both the regular focus styling (controlled by ringClasses) and focus-within styling need to be dynamically adjusted based on validation state. For proper validation styling, create separate objects for focus-within classes (like focusWithinClasses) that use the same color as ringClasses when inputInvalid=true.
Learnt from: superstes
PR: themesberg/flowbite-svelte#0
File: :0-0
Timestamp: 2025-03-20T11:52:09.106Z
Learning: In the Flowbite Svelte library's form components like MultiInput, when handling focus states for validation, avoid hardcoding focus-within classes like 'focus-within:border-primary-500'. Instead, create a dynamic mapping object (e.g., focusWithinClasses) that changes the focus ring color based on the current color state, especially when inputInvalid=true.
src/lib/badge/Badge.svelte (2)
Learnt from: mrh1997
PR: themesberg/flowbite-svelte#1442
File: src/lib/utils/Popper.svelte:144-145
Timestamp: 2024-11-22T08:37:27.996Z
Learning: In `src/lib/utils/Popper.svelte`, inconsistencies in event handling (e.g., using both `mouseenter`/`mouseleave` and `pointerenter`/`pointerleave`) may be intentional to minimize modifications and reduce the risk of new problems.
Learnt from: LahTeuto
PR: themesberg/flowbite-svelte#1484
File: src/lib/navbar/NavLi.svelte:34-34
Timestamp: 2024-11-12T10:36:34.807Z
Learning: When enhancing ARIA roles in Svelte components, avoid setting `role="presentation"` on elements that should retain the `link` role, as it can diminish ARIA role coverage and negatively affect accessibility.
src/lib/accordion/AccordionItem.svelte (2)
Learnt from: mrh1997
PR: themesberg/flowbite-svelte#1442
File: src/lib/utils/Popper.svelte:144-145
Timestamp: 2024-11-22T08:37:27.996Z
Learning: In `src/lib/utils/Popper.svelte`, inconsistencies in event handling (e.g., using both `mouseenter`/`mouseleave` and `pointerenter`/`pointerleave`) may be intentional to minimize modifications and reduce the risk of new problems.
Learnt from: LahTeuto
PR: themesberg/flowbite-svelte#1484
File: src/lib/navbar/NavLi.svelte:34-34
Timestamp: 2024-11-12T10:36:34.807Z
Learning: When enhancing ARIA roles in Svelte components, avoid setting `role="presentation"` on elements that should retain the `link` role, as it can diminish ARIA role coverage and negatively affect accessibility.
src/lib/banner/Banner.svelte (1)
Learnt from: mrh1997
PR: themesberg/flowbite-svelte#1442
File: src/lib/utils/Popper.svelte:144-145
Timestamp: 2024-11-22T08:37:27.996Z
Learning: In `src/lib/utils/Popper.svelte`, inconsistencies in event handling (e.g., using both `mouseenter`/`mouseleave` and `pointerenter`/`pointerleave`) may be intentional to minimize modifications and reduce the risk of new problems.
src/lib/types.ts (2)
Learnt from: Chizaruu
PR: themesberg/flowbite-svelte#1465
File: src/lib/forms/Timepicker.svelte:105-116
Timestamp: 2024-10-18T09:38:03.879Z
Learning: In `Timepicker.svelte`, the `Dropdown` opens as intended and is used exclusively with the 'dropdown' and 'timerange-dropdown' Timepicker types.
Learnt from: superstes
PR: themesberg/flowbite-svelte#0
File: :0-0
Timestamp: 2025-03-20T11:11:53.953Z
Learning: In the Flowbite Svelte library's MultiInput component, the 'inputInvalid' property should be exported and passed to the Wrapper component to properly style the component's ring with red color when validation fails, similar to how the Input component handles validation states.
🧬 Code Graph Analysis (4)
src/lib/banner/theme.ts (1)
src/lib/theme/themeUtils.ts (1)
Classes(11-13)
src/lib/accordion/theme.ts (2)
src/lib/accordion/index.ts (2)
accordion(3-3)accordionItem(3-3)src/lib/theme/themeUtils.ts (1)
Classes(11-13)
src/lib/theme/themeUtils.ts (2)
src/lib/index.ts (1)
getTheme(5-5)src/lib/types.ts (1)
ThemeConfig(1736-1738)
src/lib/types.ts (5)
src/lib/accordion/theme.ts (2)
AccordionVariants(5-5)AccordionItemVariants(6-6)src/lib/avatar/theme.ts (1)
AvatarVariants(4-4)src/lib/banner/theme.ts (1)
BannerVariants(5-5)src/lib/forms/helper/theme.ts (1)
HelperVariants(4-4)src/lib/toolbar/theme.ts (1)
ToolbarGroupVariants(91-91)
🪛 ESLint
src/lib/theme/themeUtils.ts
[error] 10-10: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 11-11: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 15-15: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
src/lib/types.ts
[error] 376-376: An interface declaring no members is equivalent to its supertype.
(@typescript-eslint/no-empty-object-type)
[error] 594-594: An interface declaring no members is equivalent to its supertype.
(@typescript-eslint/no-empty-object-type)
[error] 1092-1092: An interface declaring no members is equivalent to its supertype.
(@typescript-eslint/no-empty-object-type)
[error] 1437-1437: An interface declaring no members is equivalent to its supertype.
(@typescript-eslint/no-empty-object-type)
[error] 1447-1447: An interface declaring no members is equivalent to its supertype.
(@typescript-eslint/no-empty-object-type)
🔇 Additional comments (24)
src/lib/avatar/theme.ts (1)
2-4: LGTM! Good type safety improvement.The addition of
VariantPropsimport and theAvatarVariantstype alias properly extracts variant props from the avatar theme configuration, enhancing type safety and consistency across the component system.src/lib/banner/theme.ts (1)
3-5: LGTM! Proper type extension for classes prop.The addition of
Classesimport and extendingBannerVariantswithClasses<typeof banner>correctly implements the new unified styling approach, allowing slot-based class overrides through theclassesprop while maintaining type safety.src/lib/badge/theme.ts (1)
1-5: LGTM! Clean import organization and type extension.The import reorganization and extension of
BadgeVariantswithClasses<typeof badge>properly implements the unified styling approach, maintaining consistency with the broader component refactor while enhancing type safety.src/lib/accordion/theme.ts (1)
1-6: LGTM! Proper variant type definitions for accordion components.The addition of
AccordionVariantsandAccordionItemVariantstype aliases correctly extracts variant props from their respective theme configurations. The inclusion ofClasses<typeof accordionItem>inAccordionItemVariantsappropriately supports slot-based styling for the more complex accordion item component.src/lib/badge/Badge.svelte (2)
8-12: LGTM! Well-structured backward-compatible styling refactor.The prop destructuring, deprecation warning, and reactive
stylingvariable correctly implement the unified styling approach while maintaining backward compatibility with the deprecatedaClassprop.
28-28: LGTM! Proper integration of unified styling.The link class usage correctly merges theme classes with the new styling object, seamlessly supporting both the new
classesprop and the legacyaClassprop through the reactivestylingvariable.src/lib/theme/themeUtils.ts (1)
2-2: LGTM!The type-only import of
ClassValuefrom clsx is appropriate for the new class handling functionality.src/lib/banner/Banner.svelte (4)
6-8: LGTM!The import of theme utilities and the props destructuring correctly incorporate the new
classesprop while maintaining backward compatibility with deprecated props.
10-11: LGTM!The deprecation warning and styling derivation logic correctly implement the new pattern while maintaining backward compatibility. The mapping of deprecated props to their slot equivalents is accurate.
21-21: LGTM!The template correctly uses
styling.insideDivfrom the derived styling object, providing a unified interface for both new and deprecated props.
28-28: LGTM!The template correctly uses
styling.dismissablefrom the derived styling object, maintaining consistency with the new styling approach.src/lib/accordion/AccordionItem.svelte (6)
2-10: LGTM!The import reorganization and props destructuring correctly integrate the new
classesprop while maintaining backward compatibility with all deprecated styling props.
12-14: LGTM!The deprecation warning correctly covers all styling props being deprecated, and the styling object mapping is accurate. The type annotation provides good type safety.
37-37: LGTM!The buttonClass derivation correctly prioritizes the new styling approach (
styling.active/styling.inactive) while maintaining backward compatibility with context-based styling.
40-41: LGTM!The template correctly applies
classNameto the outer h2 element and usesstyling.buttonfor the button styling, implementing the new unified styling approach.
66-66: LGTM!The content div correctly uses
styling.contentfrom the unified styling object.
73-73: LGTM!The content div correctly uses
styling.contentfrom the unified styling object in the alternative branch.src/lib/types.ts (7)
69-70: LGTM!The new variant type imports correctly support the updated prop interfaces and align with the new variant-based typing approach.
160-160: LGTM!The AccordionProps interface correctly extends AccordionVariants and omits the "color" attribute to prevent conflicts with the variant's color property.
168-168: LGTM!The AccordionItemProps interface correctly extends AccordionItemVariants while maintaining HTMLAttributes support, providing access to the new classes prop.
193-193: LGTM!The AvatarProps interface correctly extends AvatarVariants while maintaining HTMLAttributes support, following the consistent pattern across component prop interfaces.
220-220: LGTM!The BannerProps interface correctly extends BannerVariants and omits the "color" attribute to prevent conflicts, maintaining consistency with other component prop interfaces.
674-676: LGTM!The formatting improvements in the CheckboxProps children snippet enhance readability without affecting functionality.
934-934: LGTM!The formatting improvement in the TimepickerProps onselect property enhances readability.
|
Is this ready for merge? |
|
It is ready for |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
src/lib/types.ts (6)
257-277: Remove commented code blockThe large block of commented code appears to be documentation showing attributes now handled by
AnchorButtonAttributes. Consider removing this commented section to keep the code clean, as the information is already conveyed through the type system.- // Common attributes that make sense for both button and anchor - // AnchorButtonDivAttributes provides them - // id?: string; - // style?: string; - // tabindex?: number; - // title?: string; - // role?: string; - // "aria-label"?: string; - // "data-testid"?: string; - // // Anchor-specific attributes - // href?: string; - // target?: string; - // rel?: string; - // download?: string | boolean; - // // Button-specific attributes - // type?: "button" | "submit" | "reset"; - // disabled?: boolean; - // name?: string; - // value?: string | number | string[]; - // // Allow any other attributes (like data-* attributes) - // [key: string]: any;
369-369: Consider converting empty interface to type aliasThe empty
CheckIconPropsinterface is equivalent to its supertypeSVGAttributes<SVGSVGElement>. Consider using a type alias instead:-export interface CheckIconProps extends SVGAttributes<SVGSVGElement> { } +export type CheckIconProps = SVGAttributes<SVGSVGElement>;
587-587: Consider converting empty interface to type aliasThe empty
DropdownDividerPropsinterface is equivalent to its supertype. Consider using a type alias:-export interface DropdownDividerProps extends HTMLAttributes<HTMLDivElement> { } +export type DropdownDividerProps = HTMLAttributes<HTMLDivElement>;
1085-1085: Consider converting empty interface to type aliasThe empty
NavBrandPropsinterface is equivalent to its supertype. Consider using a type alias:-export interface NavBrandProps extends HTMLAnchorAttributes { } +export type NavBrandProps = HTMLAnchorAttributes;
1430-1430: Consider converting empty interface to type aliasThe empty
TestimonialPlaceholderPropsinterface is equivalent to its supertype. Consider using a type alias:-export interface TestimonialPlaceholderProps extends HTMLAttributes<HTMLDivElement> { } +export type TestimonialPlaceholderProps = HTMLAttributes<HTMLDivElement>;
1440-1440: Consider converting empty interface to type aliasThe empty
WidgetPlaceholderPropsinterface is equivalent to its supertype. Consider using a type alias:-export interface WidgetPlaceholderProps extends HTMLAttributes<HTMLDivElement> { } +export type WidgetPlaceholderProps = HTMLAttributes<HTMLDivElement>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/lib/bottom-navigation/BottomNav.svelte(2 hunks)src/lib/bottom-navigation/BottomNavHeader.svelte(1 hunks)src/lib/bottom-navigation/BottomNavItem.svelte(4 hunks)src/lib/bottom-navigation/theme.ts(3 hunks)src/lib/types.ts(14 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: superstes
PR: themesberg/flowbite-svelte#0
File: :0-0
Timestamp: 2025-03-20T11:52:09.106Z
Learning: In the Flowbite Svelte library's form components, both the regular focus styling (controlled by ringClasses) and focus-within styling need to be dynamically adjusted based on validation state. For proper validation styling, create separate objects for focus-within classes (like focusWithinClasses) that use the same color as ringClasses when inputInvalid=true.
Learnt from: superstes
PR: themesberg/flowbite-svelte#0
File: :0-0
Timestamp: 2025-03-20T11:52:09.106Z
Learning: In the Flowbite Svelte library's form components like MultiInput, when handling focus states for validation, avoid hardcoding focus-within classes like 'focus-within:border-primary-500'. Instead, create a dynamic mapping object (e.g., focusWithinClasses) that changes the focus ring color based on the current color state, especially when inputInvalid=true.
src/lib/bottom-navigation/BottomNav.svelte (2)
Learnt from: mrh1997
PR: themesberg/flowbite-svelte#1442
File: src/lib/utils/Popper.svelte:144-145
Timestamp: 2024-11-22T08:37:27.996Z
Learning: In `src/lib/utils/Popper.svelte`, inconsistencies in event handling (e.g., using both `mouseenter`/`mouseleave` and `pointerenter`/`pointerleave`) may be intentional to minimize modifications and reduce the risk of new problems.
Learnt from: superstes
PR: themesberg/flowbite-svelte#0
File: :0-0
Timestamp: 2025-03-20T11:52:09.106Z
Learning: In the Flowbite Svelte library's form components, both the regular focus styling (controlled by ringClasses) and focus-within styling need to be dynamically adjusted based on validation state. For proper validation styling, create separate objects for focus-within classes (like focusWithinClasses) that use the same color as ringClasses when inputInvalid=true.
src/lib/bottom-navigation/BottomNavHeader.svelte (1)
Learnt from: mrh1997
PR: themesberg/flowbite-svelte#1442
File: src/lib/utils/Popper.svelte:144-145
Timestamp: 2024-11-22T08:37:27.996Z
Learning: In `src/lib/utils/Popper.svelte`, inconsistencies in event handling (e.g., using both `mouseenter`/`mouseleave` and `pointerenter`/`pointerleave`) may be intentional to minimize modifications and reduce the risk of new problems.
src/lib/bottom-navigation/BottomNavItem.svelte (2)
Learnt from: mrh1997
PR: themesberg/flowbite-svelte#1442
File: src/lib/utils/Popper.svelte:144-145
Timestamp: 2024-11-22T08:37:27.996Z
Learning: In `src/lib/utils/Popper.svelte`, inconsistencies in event handling (e.g., using both `mouseenter`/`mouseleave` and `pointerenter`/`pointerleave`) may be intentional to minimize modifications and reduce the risk of new problems.
Learnt from: LahTeuto
PR: themesberg/flowbite-svelte#1484
File: src/lib/navbar/NavLi.svelte:34-34
Timestamp: 2024-11-12T10:36:34.807Z
Learning: When enhancing ARIA roles in Svelte components, avoid setting `role="presentation"` on elements that should retain the `link` role, as it can diminish ARIA role coverage and negatively affect accessibility.
src/lib/types.ts (2)
Learnt from: Chizaruu
PR: themesberg/flowbite-svelte#1465
File: src/lib/forms/Timepicker.svelte:105-116
Timestamp: 2024-10-18T09:38:03.879Z
Learning: In `Timepicker.svelte`, the `Dropdown` opens as intended and is used exclusively with the 'dropdown' and 'timerange-dropdown' Timepicker types.
Learnt from: superstes
PR: themesberg/flowbite-svelte#0
File: :0-0
Timestamp: 2025-03-20T11:11:53.953Z
Learning: In the Flowbite Svelte library's MultiInput component, the 'inputInvalid' property should be exported and passed to the Wrapper component to properly style the component's ring with red color when validation fails, similar to how the Input component handles validation states.
🧬 Code Graph Analysis (2)
src/lib/bottom-navigation/theme.ts (2)
src/lib/bottom-navigation/index.ts (7)
bottomNav(5-5)bottomNavItem(5-5)BottomNavItemTheme(5-5)bottomNavHeader(5-5)BottomNavHeaderTheme(5-5)bottomNavHeaderItem(5-5)BottomNavHeaderItemTheme(5-5)src/lib/theme/themeUtils.ts (1)
Classes(11-13)
src/lib/types.ts (6)
src/lib/accordion/theme.ts (2)
AccordionVariants(5-5)AccordionItemVariants(6-6)src/lib/avatar/theme.ts (1)
AvatarVariants(4-4)src/lib/banner/theme.ts (1)
BannerVariants(5-5)src/lib/bottom-navigation/theme.ts (4)
BottomNavVariants(123-123)BottomNavItemVariants(127-127)BottomNavHeaderVariants(131-131)BottomNavHeaderItemVariants(135-135)src/lib/forms/helper/theme.ts (1)
HelperVariants(4-4)src/lib/toolbar/theme.ts (1)
ToolbarGroupVariants(91-91)
🪛 ESLint
src/lib/types.ts
[error] 369-369: An interface declaring no members is equivalent to its supertype.
(@typescript-eslint/no-empty-object-type)
[error] 587-587: An interface declaring no members is equivalent to its supertype.
(@typescript-eslint/no-empty-object-type)
[error] 1085-1085: An interface declaring no members is equivalent to its supertype.
(@typescript-eslint/no-empty-object-type)
[error] 1430-1430: An interface declaring no members is equivalent to its supertype.
(@typescript-eslint/no-empty-object-type)
[error] 1440-1440: An interface declaring no members is equivalent to its supertype.
(@typescript-eslint/no-empty-object-type)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Playwright and Lint
🔇 Additional comments (16)
src/lib/bottom-navigation/BottomNav.svelte (3)
7-7: LGTM - Import addition for deprecation supportThe import of
themeDeprecatedis correctly added to support the deprecation warnings for the old styling props.
24-24: Correct theme slot key updateThe change from
outertobasecorrectly reflects the slot key rename in the theme configuration.
31-36: Template updates align with new styling patternThe template correctly applies the new styling approach:
- Using
baseslot key (line 31) instead of the deprecatedouter- Using
classNameprop (line 31) for the outer container- Using
styling.inner(line 36) which provides the fallback logic fromclassesto deprecated propsThese changes maintain backward compatibility while implementing the new unified styling pattern.
src/lib/bottom-navigation/BottomNavHeader.svelte (2)
15-15: Correct theme slot key updateThe change from
outerDivtobasecorrectly reflects the theme simplification where the outer div slot was consolidated into the base slot.
18-19: Template correctly implements the new styling patternThe template changes properly use:
baseslot key for the outer container withclassNamepropstyling.innerDivfor the inner container, which provides fallback to the deprecatedinnerClassThe implementation is consistent with the theme simplification and maintains backward compatibility.
src/lib/bottom-navigation/theme.ts (4)
1-3: Necessary imports for type system improvementsThe addition of
VariantPropsandClassesimports is required for the new variant type definitions that combine styling variants with class overrides.
7-38: Systematic slot key rename fromoutertobaseThe slot key rename is consistently applied across all bottomNav variants. This change aligns with the component updates and provides a more semantic naming convention.
104-109: Theme simplification consolidates outer stylingThe change from
outerDivtobasesimplifies the theme structure while maintaining theinnerDivslot for inner styling. This consolidation reduces complexity and aligns with the component changes.
123-135: Enhanced type definitions with class override supportThe new variant types correctly combine
VariantPropswithClassesto support both variant selection and class overrides per slot. The exception forBottomNavHeaderItemVariants(line 135) is appropriate since it's a simple component without multiple slots.These type definitions enable better type safety and IntelliSense support for the component
classesprop.src/lib/bottom-navigation/BottomNavItem.svelte (5)
2-13: Consistent adoption of the new styling patternThe changes properly implement the new
classesprop pattern with:
- Correct deprecation warnings for both
spanClassandbtnClass- Proper fallback logic in the reactive
stylingvariable- Updated imports for the new type system
The implementation is consistent with the other bottom navigation components.
31-34: Reactive state correctly adapted to new prop structureThe
isActivecomputation properly adapts to the new prop structure by extractinghreffromrestProps. The nullish coalescing operator provides a safe fallback, and the active state logic remains functionally equivalent.
37-41: Class computation functions correctly updatedThe class computation functions properly implement the new styling pattern:
getCommonClassusesclassName ?? btnClassfor backward compatibilitygetSpanClassusesstyling.spanwhich provides the fallback logic fromclassesto deprecated propsThese changes maintain backward compatibility while adopting the new unified styling approach.
52-58: Simplified prop handling through restPropsThe removal of explicit
hrefprop handling simplifies the code by relying onrestPropsto pass through all appropriate attributes. This approach is cleaner and more maintainable.
61-71: Template correctly adapted to new prop structureThe conditional rendering logic properly adapts to the new prop structure by checking
restProps.href === undefined. This explicit check is more reliable than checking for falsy values and maintains the intended behavior.src/lib/types.ts (2)
69-71: Import statements for new variant typesThe import statements correctly bring in the new variant types that combine styling variants with class overrides support.
161-286: Consistent type system improvements across interfacesThe interfaces are properly updated to extend their respective variant types:
- Accordion components extend
AccordionVariantsandAccordionItemVariants- Avatar extends
AvatarVariants- Banner extends
BannerVariants- Bottom navigation components extend their respective variant types
The change of
BottomNavItemPropsfrom interface to type (line 248) is appropriate given its complex union withAnchorButtonAttributes.These changes provide better type safety and IntelliSense support for the new
classesprop pattern.
| let { children, header, position = "fixed", navType = "default", class: className, classes, innerClass, activeClass, activeUrl = "", ...restProps }: BottomNavProps = $props(); | ||
|
|
||
| themeDeprecated("BottomNav", { innerClass }); | ||
| let styling = $derived(classes ?? { inner: innerClass }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Good implementation of the new styling pattern
The prop destructuring and deprecation handling are well implemented. The reactive styling variable correctly falls back to the deprecated innerClass prop when classes is not provided.
However, I notice that outerClass is mentioned in the documentation comment (line 51) but is not included in the prop destructuring or deprecation warning. This suggests an inconsistency in the migration.
🏁 Script executed:
#!/bin/bash
# Check if outerClass is still being used in BottomNav components
rg -A 3 -B 3 "outerClass" src/lib/bottom-navigation/Length of output: 1019
Handle deprecated outerClass prop in BottomNav
The component’s documentation still lists outerClass, but it isn’t destructured or handled in the runtime code. To keep prop definitions and implementation in sync, we need to:
- Destructure
outerClassalongsideinnerClassin src/lib/bottom-navigation/BottomNav.svelte (around lines 9–12). - Call
themeDeprecatedforouterClass(e.g.themeDeprecated("BottomNav", { innerClass, outerClass });). - Include
outer: outerClassin the fallback forstyling:- let styling = $derived(classes ?? { inner: innerClass }); + let styling = $derived( + classes ?? { inner: innerClass, outer: outerClass } + );
This ensures the deprecated outerClass prop still works until fully removed and matches the docs.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/lib/bottom-navigation/BottomNav.svelte around lines 9 to 12, the
deprecated prop outerClass is documented but not handled in the code. To fix
this, destructure outerClass alongside innerClass from the props, update the
themeDeprecated call to include outerClass, and add outer: outerClass to the
fallback object passed to $derived for styling. This will maintain backward
compatibility with the deprecated outerClass prop.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/lib/types.ts (1)
255-274: Clean up commented code for better maintainability.The commented out properties should be removed since they are now provided by
AnchorButtonDivAttributesand the variant-based typing approach. Keeping commented code can lead to confusion and maintenance issues.- // Common attributes that make sense for both button and anchor - // AnchorButtonDivAttributes provides them - // id?: string; - // style?: string; - // tabindex?: number; - // title?: string; - // role?: string; - // "aria-label"?: string; - // "data-testid"?: string; - // // Anchor-specific attributes - // href?: string; - // target?: string; - // rel?: string; - // download?: string | boolean; - // // Button-specific attributes - // type?: "button" | "submit" | "reset"; - // disabled?: boolean; - // name?: string; - // value?: string | number | string[]; - // // Allow any other attributes (like data-* attributes) - // [key: string]: any;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/lib/bottom-navigation/BottomNav.svelte(2 hunks)src/lib/bottom-navigation/BottomNavHeader.svelte(1 hunks)src/lib/types.ts(13 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/bottom-navigation/BottomNav.svelte
- src/lib/bottom-navigation/BottomNavHeader.svelte
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: superstes
PR: themesberg/flowbite-svelte#0
File: :0-0
Timestamp: 2025-03-20T11:52:09.106Z
Learning: In the Flowbite Svelte library's form components, both the regular focus styling (controlled by ringClasses) and focus-within styling need to be dynamically adjusted based on validation state. For proper validation styling, create separate objects for focus-within classes (like focusWithinClasses) that use the same color as ringClasses when inputInvalid=true.
Learnt from: superstes
PR: themesberg/flowbite-svelte#0
File: :0-0
Timestamp: 2025-03-20T11:52:09.106Z
Learning: In the Flowbite Svelte library's form components like MultiInput, when handling focus states for validation, avoid hardcoding focus-within classes like 'focus-within:border-primary-500'. Instead, create a dynamic mapping object (e.g., focusWithinClasses) that changes the focus ring color based on the current color state, especially when inputInvalid=true.
src/lib/types.ts (2)
Learnt from: Chizaruu
PR: themesberg/flowbite-svelte#1465
File: src/lib/forms/Timepicker.svelte:105-116
Timestamp: 2024-10-18T09:38:03.879Z
Learning: In `Timepicker.svelte`, the `Dropdown` opens as intended and is used exclusively with the 'dropdown' and 'timerange-dropdown' Timepicker types.
Learnt from: superstes
PR: themesberg/flowbite-svelte#0
File: :0-0
Timestamp: 2025-03-20T11:11:53.953Z
Learning: In the Flowbite Svelte library's MultiInput component, the 'inputInvalid' property should be exported and passed to the Wrapper component to properly style the component's ring with red color when validation fails, similar to how the Input component handles validation states.
🪛 ESLint
src/lib/types.ts
[error] 367-367: An interface declaring no members is equivalent to its supertype.
(@typescript-eslint/no-empty-object-type)
[error] 585-585: An interface declaring no members is equivalent to its supertype.
(@typescript-eslint/no-empty-object-type)
[error] 1083-1083: An interface declaring no members is equivalent to its supertype.
(@typescript-eslint/no-empty-object-type)
[error] 1428-1428: An interface declaring no members is equivalent to its supertype.
(@typescript-eslint/no-empty-object-type)
[error] 1438-1438: An interface declaring no members is equivalent to its supertype.
(@typescript-eslint/no-empty-object-type)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Playwright and Lint
🔇 Additional comments (13)
src/lib/types.ts (13)
69-71: LGTM! New variant imports align with the unified classes prop approach.The new variant type imports for accordion, avatar, and bottom navigation components properly support the transition to variant-based typing mentioned in the PR objectives.
161-161: LGTM! AccordionProps correctly extends AccordionVariants.The interface now properly uses variant-based typing, which aligns with the PR's goal of introducing a unified classes prop approach.
169-169: LGTM! AccordionItemProps correctly extends AccordionItemVariants.The interface properly adopts the variant-based typing approach consistent with the PR objectives.
194-194: LGTM! AvatarProps correctly extends AvatarVariants.The interface now uses variant-based typing, removing redundant explicit styling properties mentioned in the AI summary.
221-221: LGTM! BannerProps correctly extends BannerVariants.The interface properly adopts variant-based typing, supporting the new classes prop pattern mentioned in the PR objectives.
238-238: LGTM! BottomNavProps correctly extends BottomNavVariants.The interface now uses variant-based typing, consistent with the bottom navigation component refactoring mentioned in the PR.
247-247: LGTM! BottomNavItemProps correctly extends BottomNavItemVariants.The type properly adopts variant-based typing, aligning with the bottom navigation component refactoring.
278-278: LGTM! BottomNavHeaderProps correctly extends BottomNavHeaderVariants.The interface properly adopts variant-based typing, consistent with the bottom navigation component refactoring.
284-284: LGTM! BottomNavHeaderItemProps correctly extends BottomNavHeaderItemVariants.The interface properly adopts variant-based typing, completing the bottom navigation component type updates.
665-667: LGTM! Improved type definition formatting.The restructured type definition improves readability while maintaining the same functionality.
739-739: LGTM! HelperProps correctly extends HelperVariants.The interface properly adopts variant-based typing, consistent with the overall refactoring approach.
925-925: LGTM! Enhanced callback flexibility.The addition of
[key: string]: stringto the onselect callback parameter allows for more flexible data passing, which is a reasonable enhancement.
1119-1119: LGTM! ToolbarGroupProps correctly extends ToolbarGroupVariants.The interface properly adopts variant-based typing, consistent with the overall refactoring approach.
| } | ||
|
|
||
| export interface NavBrandProps extends HTMLAnchorAttributes {} | ||
| export interface NavBrandProps extends HTMLAnchorAttributes { } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant empty interface.
The NavBrandProps interface is equivalent to its supertype HTMLAnchorAttributes. Consider removing this interface and using the base type directly, or add meaningful properties if needed.
-export interface NavBrandProps extends HTMLAnchorAttributes { }Then update any usage to use HTMLAnchorAttributes directly.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 ESLint
[error] 1083-1083: An interface declaring no members is equivalent to its supertype.
(@typescript-eslint/no-empty-object-type)
🤖 Prompt for AI Agents
In src/lib/types.ts at line 1083, the NavBrandProps interface is redundant as it
only extends HTMLAnchorAttributes without adding any new properties. Remove the
NavBrandProps interface entirely and update all references in the codebase to
use HTMLAnchorAttributes directly instead.
| } | ||
|
|
||
| export interface TestimonialPlaceholderProps extends HTMLAttributes<HTMLDivElement> {} | ||
| export interface TestimonialPlaceholderProps extends HTMLAttributes<HTMLDivElement> { } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant empty interface.
The TestimonialPlaceholderProps interface is equivalent to its supertype HTMLAttributes<HTMLDivElement>. Consider removing this interface and using the base type directly, or add meaningful properties if needed.
-export interface TestimonialPlaceholderProps extends HTMLAttributes<HTMLDivElement> { }Then update any usage to use HTMLAttributes<HTMLDivElement> directly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export interface TestimonialPlaceholderProps extends HTMLAttributes<HTMLDivElement> { } | |
| // (Interface removed; use HTMLAttributes<HTMLDivElement> directly) |
🧰 Tools
🪛 ESLint
[error] 1428-1428: An interface declaring no members is equivalent to its supertype.
(@typescript-eslint/no-empty-object-type)
🤖 Prompt for AI Agents
In src/lib/types.ts at line 1428, the interface TestimonialPlaceholderProps is
redundant as it only extends HTMLAttributes<HTMLDivElement> without adding any
new properties. Remove the TestimonialPlaceholderProps interface entirely and
update all references in the codebase to use HTMLAttributes<HTMLDivElement>
directly instead.
| } | ||
|
|
||
| export interface CheckIconProps extends SVGAttributes<SVGSVGElement> {} | ||
| export interface CheckIconProps extends SVGAttributes<SVGSVGElement> { } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant empty interface.
The CheckIconProps interface is equivalent to its supertype SVGAttributes<SVGSVGElement>. Consider removing this interface and using the base type directly, or add meaningful properties if needed.
-export interface CheckIconProps extends SVGAttributes<SVGSVGElement> { }Then update any usage to use SVGAttributes<SVGSVGElement> directly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export interface CheckIconProps extends SVGAttributes<SVGSVGElement> { } | |
| // (The redundant CheckIconProps interface has been removed. Update any references to use SVGAttributes<SVGSVGElement> directly.) |
🧰 Tools
🪛 ESLint
[error] 367-367: An interface declaring no members is equivalent to its supertype.
(@typescript-eslint/no-empty-object-type)
🤖 Prompt for AI Agents
In src/lib/types.ts at line 367, the CheckIconProps interface is redundant as it
only extends SVGAttributes<SVGSVGElement> without adding any new properties.
Remove the CheckIconProps interface entirely and update all references in the
codebase to use SVGAttributes<SVGSVGElement> directly instead.
| } | ||
|
|
||
| export interface WidgetPlaceholderProps extends HTMLAttributes<HTMLDivElement> {} | ||
| export interface WidgetPlaceholderProps extends HTMLAttributes<HTMLDivElement> { } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant empty interface.
The WidgetPlaceholderProps interface is equivalent to its supertype HTMLAttributes<HTMLDivElement>. Consider removing this interface and using the base type directly, or add meaningful properties if needed.
-export interface WidgetPlaceholderProps extends HTMLAttributes<HTMLDivElement> { }Then update any usage to use HTMLAttributes<HTMLDivElement> directly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export interface WidgetPlaceholderProps extends HTMLAttributes<HTMLDivElement> { } |
🧰 Tools
🪛 ESLint
[error] 1438-1438: An interface declaring no members is equivalent to its supertype.
(@typescript-eslint/no-empty-object-type)
🤖 Prompt for AI Agents
In src/lib/types.ts at line 1438, the WidgetPlaceholderProps interface is
redundant as it only extends HTMLAttributes<HTMLDivElement> without adding any
new properties. Remove the WidgetPlaceholderProps interface entirely and update
all references in the codebase to use HTMLAttributes<HTMLDivElement> directly
instead.
| } | ||
|
|
||
| export interface DropdownDividerProps extends HTMLAttributes<HTMLDivElement> {} | ||
| export interface DropdownDividerProps extends HTMLAttributes<HTMLDivElement> { } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant empty interface.
The DropdownDividerProps interface is equivalent to its supertype HTMLAttributes<HTMLDivElement>. Consider removing this interface and using the base type directly, or add meaningful properties if needed.
-export interface DropdownDividerProps extends HTMLAttributes<HTMLDivElement> { }Then update any usage to use HTMLAttributes<HTMLDivElement> directly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export interface DropdownDividerProps extends HTMLAttributes<HTMLDivElement> { } |
🧰 Tools
🪛 ESLint
[error] 585-585: An interface declaring no members is equivalent to its supertype.
(@typescript-eslint/no-empty-object-type)
🤖 Prompt for AI Agents
In src/lib/types.ts at line 585, the interface DropdownDividerProps is redundant
as it only extends HTMLAttributes<HTMLDivElement> without adding any new
properties. Remove the DropdownDividerProps interface entirely and update all
references in the codebase to use HTMLAttributes<HTMLDivElement> directly
instead.
2 participants
📑 Description
First approach to
classes.Adds
classeswithout removing individualxxxClassprops, however it console logs deprecation warning.Accordion,Badge,Banner,BottomNavWe can introduce those changes partially, so I think it can be merged.
Status
✅ Checks
ℹ Additional Information
Summary by CodeRabbit
New Features
classesprop for AccordionItem, Badge, Banner, BottomNav, BottomNavHeader, and BottomNavItem components to simplify and modernize styling customization.classesprop, with warnings for deprecated usage.Refactor
outertobasein BottomNav and BottomNavHeader themes for consistent slot naming.classesprop.Documentation
classesprop.Style