Feature/custom dropdown classes #339
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces several enhancements to the project's components and configuration. The changes primarily focus on improving type flexibility in the Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
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: 1
🧹 Nitpick comments (4)
src/components/FwbDropdown/composables/useDropdownClasses.ts (1)
81-85: Potential performance implications of usingMutationObserverwithsubtree: trueObserving mutations with
{ childList: true, subtree: true }can impact performance, especially if the observed content is large or frequently changing. Consider limiting the observer to essential mutations or finding alternative ways to detect necessary changes.src/components/FwbDropdown/FwbDropdown.vue (3)
10-11: Simplify conditional class bindings for better readabilityCurrently, both
'flex-row-reverse'and'pl-2'classes are conditionally applied whenplacement === 'left'. You can streamline this by grouping the classes together, improving readability.Apply this diff:
<fwb-button - :class="{'flex-row-reverse': placement === 'left', 'pl-2': placement === 'left'}" + :class="[placement === 'left' ? ['flex-row-reverse', 'pl-2'] : '', props.triggerClass]" :color="color" :disabled="disabled" >
99-101: Avoid assignments within logical expressions for clarityUsing an assignment within a logical
&&can reduce code readability. Refactor to make the code more explicit and maintainable.Apply this diff:
onClickOutside(dropdownWrapper, () => - isContentVisible.value && (isContentVisible.value = false), + { + if (isContentVisible.value) { + isContentVisible.value = false + } + }, )
108-112: Refactor thewatchcallback for better readabilityUsing a ternary operator inside the
watchcallback may hinder clarity. Consider using an explicitif...elsestatement to enhance readability.Apply this diff:
watch(isContentVisible, () => { - isContentVisible.value - ? emit('show') - : emit('hide') + if (isContentVisible.value) { + emit('show') + } else { + emit('hide') + } })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.gitignore(1 hunks)src/components/FwbButton/FwbButton.vue(1 hunks)src/components/FwbButton/composables/useButtonClasses.ts(1 hunks)src/components/FwbDropdown/FwbDropdown.vue(4 hunks)src/components/FwbDropdown/composables/useDropdownClasses.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🔇 Additional comments (4)
src/components/FwbButton/FwbButton.vue (1)
87-87: LGTM! Enhanced type flexibility for class binding.The change to support both string and object syntax for the class property aligns with Vue's class binding capabilities, offering more flexibility while maintaining type safety.
src/components/FwbButton/composables/useButtonClasses.ts (2)
147-148: LGTM! Consistent type definitions.The change to interface and updated class property type maintains consistency with the button component's changes while providing better extensibility.
Line range hint
171-171: Address TODO comments for error handling.There are TODO comments about prettifying console warnings. Consider implementing proper error handling or documentation for these cases:
- When using outline prop with simple gradients
- When using outline prop with alternative colors
Would you like me to help implement a proper error handling solution or create an issue to track this improvement?
Also applies to: 182-182
src/components/FwbDropdown/composables/useDropdownClasses.ts (1)
Line range hint
37-117: LGTM!The refactored
useDropdownClassescomposable enhances modularity and improves the clarity of dropdown class management. The changes make the component more flexible and maintainable.
| triggerClass?: string | ||
| triggerWrapperClass?: string |
There was a problem hiding this comment.
The triggerClass prop is defined but not utilized
The triggerClass prop is declared in DropdownProps and provided with a default value; however, it's not applied in the template. This omission prevents users from customizing the trigger button's classes as intended.
Apply this diff to include triggerClass in the <fwb-button> component:
<fwb-button
- :class="{'flex-row-reverse': placement === 'left', 'pl-2': placement === 'left'}"
+ :class="[placement === 'left' ? ['flex-row-reverse', 'pl-2'] : '', props.triggerClass]"
:color="color"
:disabled="disabled"
>Committable suggestion skipped: line range outside the PR's diff.
| @@ -84,7 +84,7 @@ import { useButtonSpinner } from './composables/useButtonSpinner' | |||
| import type { ButtonGradient, ButtonMonochromeGradient, ButtonSize, ButtonVariant } from './types' | |||
|
|
|||
| interface IButtonProps { | |||
| class?: string | |||
| class?: string | object | |||
There was a problem hiding this comment.
this is required to allow dynamic binding of user classes
| <fwb-slot-listener @click="onToggle"> | ||
| <slot name="trigger"> | ||
| <fwb-button | ||
| :disabled="disabled" | ||
| :class="{'flex-row-reverse': placement === 'left', 'pl-2': placement === 'left'}" |
There was a problem hiding this comment.
this fixes padding and margin for left placed dropdown... this view:
| :class="triggerSuffixClass" | ||
| class="w-4 h-4" |
There was a problem hiding this comment.
this is another part for the "left placement"
| v-if="visible" | ||
| ref="content" | ||
| :class="[contentClasses]" | ||
| v-if="isContentVisible" |
There was a problem hiding this comment.
variable renamed to better match it's use
| ref="content" | ||
| :class="[contentClasses]" | ||
| v-if="isContentVisible" | ||
| ref="contentWrapper" |
There was a problem hiding this comment.
ref renamed to better match what it is referring to
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/FwbDropdown/FwbDropdown.vue (1)
108-111: Consider simplifying the watch effect.The watch effect can be more concise using the ternary operator.
Apply this diff to simplify:
-watch(isContentVisible, () => { - isContentVisible.value - ? emit('show') - : emit('hide') -}) +watch(isContentVisible, (value) => emit(value ? 'show' : 'hide'))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/FwbDropdown/FwbDropdown.vue(4 hunks)
🔇 Additional comments (4)
src/components/FwbDropdown/FwbDropdown.vue (4)
3-4: LGTM! Improved naming clarity.The ref renaming from 'wrapper' to 'dropdownWrapper' and the use of computed 'wrapperClasses' improves code readability and maintainability.
61-72: LGTM! Well-structured props interface.The DropdownProps interface is well-organized and provides good type safety for the component's configuration options.
137-201: LGTM! Well-structured transition animations.The transition styles are well-organized, consistently implemented, and properly scoped under the .fwb-dropdown class.
10-10:⚠️ Potential issueFix object syntax in class binding.
The class binding has a syntax error in combining static and dynamic classes.
Apply this diff to fix the syntax:
- :class="{'flex-row-reverse': placement === 'left', 'pl-2': placement === 'left', triggerClass}" + :class="[{'flex-row-reverse': placement === 'left', 'pl-2': placement === 'left'}, triggerClass]"Likely invalid or redundant comment.
Sqrcz
added
📚 documentation
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
docs/components/dropdown/examples/FwbDropdownExampleTrigger.vue (1)
5-14: Add aria-label to improve accessibility.The button trigger should include an aria-label to provide context for screen readers.
- <fwb-button color="light"> + <fwb-button color="light" aria-label="Open dropdown menu">docs/components/dropdown/examples/FwbDropdownExampleListGroup.vue (1)
7-19: Enhance accessibility and maintainability.Consider the following improvements:
- The repeated class string could be extracted to reduce duplication
- Add ARIA attributes for better accessibility
- Add click handlers for the navigation actions
Here's a suggested implementation:
+ <script setup> + import { FwbDropdown, FwbListGroup, FwbListGroupItem } from '../../../../src/index' + + const itemClass = 'cursor-pointer hover:bg-gray-100 dark:hover:bg-gray-600 dark:hover:text-white' + </script> + <template> <div class="vp-raw"> <fwb-dropdown text="Menu" content-class="rounded-lg" > - <fwb-list-group class="text-sm text-gray-700 dark:text-gray-200"> + <fwb-list-group + class="text-sm text-gray-700 dark:text-gray-200" + role="menu" + > <fwb-list-group-item - class="cursor-pointer hover:bg-gray-100 dark:hover:bg-gray-600 dark:hover:text-white" + :class="itemClass" + role="menuitem" + @click="() => {/* handle dashboard navigation */}" > Dashboard </fwb-list-group-item> <!-- Apply similar changes to other items --> </fwb-list-group> </fwb-dropdown> </div> </template>docs/components/dropdown/examples/FwbDropdownExampleDisabled.vue (1)
4-21: Enhance accessibility and reduce code duplicationThe dropdown menus need accessibility improvements and have duplicated navigation structures:
- Add
aria-labelto distinguish between normal and disabled state menus- Consider using
role="menuitem"for dropdown items- Extract the repeated navigation structure into a reusable component
Example improvement for the first dropdown:
- <nav class="py-2 text-sm text-gray-700 dark:text-gray-200"> + <nav aria-label="Dropdown navigation" class="py-2 text-sm text-gray-700 dark:text-gray-200"> <a href="#" + role="menuitem" class="block px-4 py-2 hover:bg-gray-100 dark:hover:bg-gray-600 dark:hover:text-white" >Dashboard</a>Consider creating a reusable component to reduce duplication:
<!-- DropdownMenu.vue --> <template> <nav :aria-label="label" class="py-2 text-sm text-gray-700 dark:text-gray-200"> <a v-for="item in items" :key="item" href="#" role="menuitem" class="block px-4 py-2 hover:bg-gray-100 dark:hover:bg-gray-600 dark:hover:text-white" >{{ item }}</a> </nav> </template>Also applies to: 27-44
docs/components/dropdown/examples/FwbDropdownExampleAlignment.vue (1)
1-1: Consider architectural improvements for dropdown systemTo improve maintainability and consistency across the dropdown system:
- Create a shared dropdown menu component
- Implement proper keyboard navigation
- Add proper ARIA attributes and roles
- Consider using a proper routing solution
Would you like me to provide a complete implementation of these improvements?
src/components/FwbDropdown/FwbDropdown.vue (2)
61-72: Well-structured props interface with clear type definitions.The
DropdownPropsinterface is well-organized and provides good type safety. However, consider adding JSDoc comments for better documentation.Add JSDoc comments to improve prop documentation:
export interface DropdownProps { + /** Reverses the alignment of the dropdown content */ alignToEnd?: boolean + /** Additional classes for the main dropdown wrapper */ class?: string + /** Enable closing dropdown when clicking inside content */ closeInside?: boolean + /** Button variant for the default trigger */ color?: ButtonVariant + /** Additional classes for the content wrapper */ contentWrapperClass?: string + /** Disable the dropdown */ disabled?: boolean + /** Dropdown content placement */ placement?: DropdownPlacement + /** Button text for the default trigger */ text?: string + /** Custom transition name */ transition?: string + /** Additional classes for the trigger button */ triggerClass?: string + /** Additional classes for the trigger wrapper */ triggerWrapperClass?: string }
108-111: Consider using TypeScript's strict event typing.The event emission could benefit from stricter typing.
Consider using a more specific event type:
-const emit = defineEmits<{ - show: [] - hide: [] -}>() +const emit = defineEmits<{ + /** Emitted when dropdown is shown */ + (e: 'show'): void + /** Emitted when dropdown is hidden */ + (e: 'hide'): void +}>()docs/components/dropdown.md (1)
148-148: Add language specifiers to code blocks.Some code blocks are missing language specifiers, which affects syntax highlighting.
Add language specifiers to the code blocks:
- Line 148: Add
vue- Line 233: Add
vue- Line 285: Add
vueAlso applies to: 233-233, 285-285
🧰 Tools
🪛 Markdownlint (0.37.0)
148-148: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
docs/components/dropdown.md(5 hunks)docs/components/dropdown/examples/FwbDropdownExampleAlignment.vue(1 hunks)docs/components/dropdown/examples/FwbDropdownExampleButtonColors.vue(1 hunks)docs/components/dropdown/examples/FwbDropdownExampleButtonGroup.vue(1 hunks)docs/components/dropdown/examples/FwbDropdownExampleCloseInside.vue(1 hunks)docs/components/dropdown/examples/FwbDropdownExampleColors.vue(0 hunks)docs/components/dropdown/examples/FwbDropdownExampleDisabled.vue(1 hunks)docs/components/dropdown/examples/FwbDropdownExampleListGroup.vue(1 hunks)docs/components/dropdown/examples/FwbDropdownExamplePlacement.vue(1 hunks)docs/components/dropdown/examples/FwbDropdownExampleTrigger.vue(1 hunks)src/components/FwbDropdown/FwbDropdown.vue(4 hunks)
💤 Files with no reviewable changes (1)
- docs/components/dropdown/examples/FwbDropdownExampleColors.vue
🧰 Additional context used
🪛 Markdownlint (0.37.0)
docs/components/dropdown.md
148-148: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
233-233: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
285-285: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
317-317: null
Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
334-334: null
Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
340-340: null
Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
🪛 LanguageTool
docs/components/dropdown.md
[uncategorized] ~259-~259: Possible missing article found.
Context: ...andler. ::: ## Dropdown - trigger Use dedicated #trigger slot to change trigger eleme...
(AI_HYDRA_LEO_MISSING_A)
[uncategorized] ~262-~262: You might be missing the article “the” here.
Context: ...ing to your needs. ::: tip You can use triggerWrapperClass prop if you need to...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[grammar] ~323-~323: Did you mean “adding”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun.
Context: ... | | contentWrapperClass | Allows to add or override classes to content wrapper ...
(ALLOW_TO)
[grammar] ~328-~328: Did you mean “adding”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun.
Context: ...ement` | | triggerClass | Allows to add or override classes to trigger Button<b...
(ALLOW_TO)
[grammar] ~329-~329: Did you mean “adding”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun.
Context: ... | | triggerWrapperClass | Allows to add or override classes to trigger wrapper ...
(ALLOW_TO)
🔇 Additional comments (4)
docs/components/dropdown/examples/FwbDropdownExampleTrigger.vue (1)
39-39: LGTM!The imports are correctly updated to reflect the actual components being used in the template.
docs/components/dropdown/examples/FwbDropdownExamplePlacement.vue (1)
7-90: Similar improvements needed as previous filesThe same accessibility and code duplication concerns apply to this file.
docs/components/dropdown/examples/FwbDropdownExampleButtonColors.vue (1)
7-24: Improve accessibility and reduce code duplication in navigation menus.The navigation links need ARIA labels for better accessibility, and the repeated menu structure could be extracted into a reusable component.
Also applies to: 30-47, 53-70, 76-93, 99-116, 122-139, 145-162, 168-185, 191-208
src/components/FwbDropdown/FwbDropdown.vue (1)
3-4: Improved naming for better code clarity.The renaming of refs from
wrappertodropdownWrapperandvisibletoisContentVisibleimproves code readability by making their purposes more explicit.Also applies to: 38-39
docs/components/dropdown/examples/FwbDropdownExampleListGroup.vue
Outdated
Show resolved
Hide resolved
docs/components/dropdown/examples/FwbDropdownExampleAlignment.vue
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/components/FwbDropdown/FwbDropdown.vue (3)
95-104: Consider using a more descriptive name for the click handler.The
onTogglename could be more specific about what it's toggling.-const onToggle = () => (isContentVisible.value = !isContentVisible.value) +const onToggleDropdown = () => (isContentVisible.value = !isContentVisible.value)Also update the template:
- <fwb-slot-listener @click="onToggle"> + <fwb-slot-listener @click="onToggleDropdown">
111-114: Consider adding debounce to show/hide events.The watch handler might trigger events too frequently during rapid toggling.
+import { debounce } from '@vueuse/core' +const debouncedEmit = debounce((event: 'show' | 'hide') => emit(event), 250) watch(isContentVisible, () => { isContentVisible.value - ? emit('show') - : emit('hide') + ? debouncedEmit('show') + : debouncedEmit('hide') })
140-204: Consider scoping the transition styles.While the transitions are well-implemented, consider scoping them to avoid potential conflicts with other components using similar class names.
-<style> +<style scoped> .fwb-dropdown { /* transitions */ .to-bottom-enter-active, /* ... rest of the styles ... */ }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/FwbDropdown/FwbDropdown.vue(4 hunks)
🔇 Additional comments (4)
src/components/FwbDropdown/FwbDropdown.vue (4)
3-4: LGTM! Improved naming and class binding.The renaming of
wrappertodropdownWrapperand the use ofwrapperClassesimproves code clarity and maintainability.
10-15: LGTM! Enhanced accessibility with ARIA attributes.The addition of ARIA attributes (
aria-expanded,aria-haspopup, androle) improves accessibility. The class binding for left placement is also well-implemented.
41-43: LGTM! Improved state management and class structure.The renaming of
visibletoisContentVisibleand the use ofcontentWrapperref withcontentWrapperClassesimproves code organization.
64-75: LGTM! Well-structured props interface.The
DropdownPropsinterface provides good type safety and documents the component's API clearly.
|
Hey @tho-dan 👋 Since you were working on dropdown recently... Can you please take a look at the changes in this PR and confirm that everything that you've implemented recently still works ;) Would be great if you could test this component as it was extensively refactored 🙈 You can use development preview that we have here: https://deploy-preview-339--sensational-seahorse-8635f8.netlify.app/ Thank you 🙏 |
There was a problem hiding this comment.
Hi @Sqrcz, thanks so much for involving me in this! 🙏
Great to see updates to the dropdown component. This will bring a lot of flexibility into usage while keeping the code cleaner with better naming! 🚀
After going through the code and testing both on deployment preview and locally, I can confirm that everything listed in the documentation is working just like I would expect.
I added a PR to your personal repo with some minor and typo fixes, feel free to take a look at it.
| <a | ||
| href="#" | ||
| class="block px-4 py-2 hover:bg-gray-100 dark:hover:bg-gray-600 dark:hover:text-white" | ||
| >Dashboard</a> |
There was a problem hiding this comment.
The functionality of the align-to-end prop is not very obvious here, as the button is just as long as the content (for the "Bottom" button). I'd suggest using some longer content here to better visualize this, what would you think?
I created a pull request to your source branch with a suggestion
There was a problem hiding this comment.
Hey,
Thank you very much for the review and proposed changes... I've merged them and will merge this one now.
…different alignment options
Fixed some typos
Summary of main changes:
classon the component instancecontentWrapperClasstriggerWrapperClasstriggerClass(used only for default trigger)Summary by CodeRabbit
Release Notes
New Features
Chores
.gitignoreto exclude VSCode configuration files.Style
The changes focus on improving component flexibility, type safety, and user experience across the project.