Add support for grouping dropdowns and buttons together in button group #331
Conversation
WalkthroughThe changes in this pull request involve updates to the styling of buttons within the Changes
Possibly related issues
Possibly related PRs
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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. |
![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
🧹 Outside diff range and nitpick comments (2)
src/components/FwbButtonGroup/FwbButtonGroup.vue (2)
21-22: LGTM! Right edge styling properly handledThe right rounded corner is correctly applied whether the last item is a direct button or a dropdown.
Consider documenting any limitations regarding nested button groups or complex dropdown arrangements, as these might affect the border radius behavior in unexpected ways.
11-22: Consider adding visual regression testsThe styling changes look good, but given the visual nature of these changes, consider adding visual regression tests to ensure the button group appearance remains consistent across different combinations of buttons and dropdowns.
Would you like help setting up visual regression tests for these components?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/components/FwbButtonGroup/FwbButtonGroup.vue(1 hunks)src/components/FwbDropdown/FwbDropdown.vue(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/components/FwbDropdown/FwbDropdown.vue
🔇 Additional comments (2)
src/components/FwbButtonGroup/FwbButtonGroup.vue (2)
11-12: LGTM! Base button styles properly extended
The selector extension to include buttons within dropdowns ensures consistent styling across the button group.
16-17: LGTM! Left edge styling properly handled
The left rounded corner is correctly applied whether the first item is a direct button or a dropdown.
Let's verify the component usage pattern:
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
docs/components/buttonGroup/examples/FwbButtonGroupExampleDropdown.vue (1)
3-22: Add ARIA labels for better accessibilityThe button group implementation looks good, but could benefit from improved accessibility.
Consider applying these changes:
- <fwb-button-group> + <fwb-button-group aria-label="Button group with dropdowns"> - <fwb-button> Button 1 </fwb-button> - <fwb-button> Button 2 </fwb-button> + <fwb-button aria-label="Button 1"> Button 1 </fwb-button> + <fwb-button aria-label="Button 2"> Button 2 </fwb-button>docs/components/button-group.md (3)
64-64: Enhance the documentation with more details.Consider expanding the explanation to include:
- Use cases for combining buttons and dropdowns
- Available configuration options
- Best practices for implementation
77-86: Improve accessibility and usability of the list group example.The list group implementation has several areas for improvement:
- Empty href attributes in anchor tags should be replaced with meaningful URLs or button elements
- Missing ARIA attributes for better screen reader support
- Consider adding more descriptive text content instead of placeholder text
Example improvement:
<fwb-list-group> <fwb-list-group-item - v-for="item in ['This', 'is', 'a', 'list']" + v-for="item in [ + { text: 'Profile settings', href: '#settings' }, + { text: 'Messages', href: '#messages' }, + { text: 'Download data', href: '#download' } + ]" :key="item" hover > - <a href="#">{{ item }}</a> + <a :href="item.href" role="menuitem">{{ item.text }}</a> </fwb-list-group-item> </fwb-list-group>
71-75: Consider enhancing the first dropdown example.The current example uses a simple paragraph with test text. Consider showcasing a more practical use case that demonstrates real-world application.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
docs/components/button-group.md(2 hunks)docs/components/buttonGroup/examples/FwbButtonGroupExampleDropdown.vue(1 hunks)
🔇 Additional comments (3)
docs/components/buttonGroup/examples/FwbButtonGroupExampleDropdown.vue (1)
27-27: LGTM: Component imports are correctly structured
The imports are properly organized and use the correct path to the source index.
docs/components/button-group.md (2)
2-4: LGTM! Import statements are well-organized.
The imports follow a consistent naming convention and properly include the new dropdown example component.
4-4: Verify the example component implementation.
Ensure that the FwbButtonGroupExampleDropdown component is properly implemented in the examples directory.
Also applies to: 65-65
✅ Verification successful
Component implementation verified successfully
The FwbButtonGroupExampleDropdown component is properly implemented with:
- Correct imports from the library
- Valid template structure using
FwbButtonGroupwith buttons and dropdowns - Working example showcasing button group with dropdown functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the example component exists and verify its implementation
fd "FwbButtonGroupExampleDropdown.vue" --type f
Length of output: 118
Script:
#!/bin/bash
# Check the content of the example component to verify its implementation
cat docs/components/buttonGroup/examples/FwbButtonGroupExampleDropdown.vue
Length of output: 863
docs/components/buttonGroup/examples/FwbButtonGroupExampleDropdown.vue
Outdated
Show resolved
Hide resolved
|
fixes #330 |
Sqrcz
left a comment
Sqrcz
left a comment
There was a problem hiding this comment.
Very much needed addition, thank you.
Please fix that one small issue and we'll be ready to merge it.
Sqrcz
left a comment
Sqrcz
left a comment
There was a problem hiding this comment.
Another good one,
Thank you!
2 participants
Summary by CodeRabbit
New Features
Bug Fixes