-
Notifications
You must be signed in to change notification settings - Fork 136
feat(list-group-item): add active prop #370
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
Conversation
WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FwbListGroupItem
participant useListGroupItemClasses
User->>FwbListGroupItem: Set props (active, hover, disabled)
FwbListGroupItem->>useListGroupItemClasses: Pass props (active, hover, disabled)
useListGroupItemClasses-->>FwbListGroupItem: Compute itemClasses (apply active styles if active and not disabled)
FwbListGroupItem-->>User: Render with computed classes reflecting active state
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
✅ Deploy Preview for sensational-seahorse-8635f8 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/components/list-group.md (1)
45-45: Documentation updated correctly to reflect new functionality.The hover example now includes the
activeattribute on the first item, which aligns with the component example and demonstrates the new functionality to users.Consider adding a brief note in the documentation that explains the active state and its priority over hover styling, perhaps in a new section specifically for the active state.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/components/list-group.md(1 hunks)docs/components/listGroup/examples/FwbListGroupExampleHover.vue(1 hunks)src/components/FwbListGroup/FwbListGroupItem.vue(1 hunks)src/components/FwbListGroup/composables/useListGroupItemClasses.ts(2 hunks)
🔇 Additional comments (5)
src/components/FwbListGroup/composables/useListGroupItemClasses.ts (3)
8-8: Good choice of styling for active items.The Tailwind classes selected for active items (
text-white bg-blue-700 dark:bg-gray-800 cursor-pointer) align well with Flowbite's design system. The white text on blue background creates appropriate visual contrast for the active state.
13-13: LGTM! Good addition of active prop to the type definition.Adding
activeto theUseListGroupItemClassesPropstype correctly extends the component's API.
23-24: Well-implemented styling priority logic.The implementation correctly prioritizes active styling over hover styling by:
- Only applying hover styles when the item is not active and not disabled
- Applying active styles when the item is active and not disabled
This ensures a proper visual hierarchy when states are combined.
docs/components/listGroup/examples/FwbListGroupExampleHover.vue (1)
4-7: Good demonstration of the active state.The first list item now properly showcases the active state alongside hover. The multi-line format also improves code readability.
src/components/FwbListGroup/FwbListGroupItem.vue (1)
33-36: Prop implementation follows Vue best practices.The
activeprop is correctly implemented with:
- Appropriate Boolean type
- Default value of false
- Consistent style with other related props (hover, disabled)
This provides a clean API for component users.
- refactored to use newer `useMergeClasses` composable
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.
Very nice addition. Good catch.
I've updated useListGroupClasses composable to use newer approach with useMergeClasses instead of simplifyTailwindClasses.
@Flambe Please test if it still works as intended... ;)
|
@Sqrcz looks good in the netlify deploy! Thanks for the quick response :) |
The active prop from list groups was missing so this PR adds it as a prop
Summary by CodeRabbit