[Style] Add custom scrollbar styling for SelectBox components #5879
Conversation
dosubot
bot
added
the
size:M
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 10/01/2025, 12:30:58 PM UTC 🔗 Links🎉 Your Storybook is ready for review! |
🎭 Playwright Test Results⏰ Completed at: 10/01/2025, 12:46:14 PM UTC 📈 Summary
📊 Test Reports by Browser
🎉 Click on the links above to view detailed test results for each browser configuration. |
christian-byrne
added
the
claude-review
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| /* Scrollbar tokens (colors reuse existing palette) */ | ||
| --scrollbar-size: 10px; /* width/height for webkit scrollbars */ | ||
| --scrollbar-track: var(--color-white); |
There was a problem hiding this comment.
[quality] high Priority
Issue: Undefined CSS color token dependency
Context: The scrollbar implementation uses --color-white token which is not defined anywhere in the CSS. This will cause the scrollbar to fall back to browser defaults.
Suggestion: Use --color-pure-white which is defined at line 116, or define --color-white token in the color palette section.
| } | ||
|
|
||
| /* Dark theme overrides (scoped to your `.dark-theme` root) */ | ||
| .dark-theme .custom-scrollbar { |
There was a problem hiding this comment.
[architecture] medium Priority
Issue: Redundant CSS rule duplication between theme variants
Context: The dark theme scrollbar styles are repeated both in @media query and .dark-theme class, creating maintenance overhead and potential conflicts.
Suggestion: Extract scrollbar dark theme variables into a mixin or consolidate the theme overrides into the existing .dark-theme section.
| --scrollbar-size: 10px; /* width/height for webkit scrollbars */ | ||
| --scrollbar-track: var(--color-white); | ||
| --scrollbar-thumb: var(--color-gray-400); | ||
| --scrollbar-thumb-hover: var(--color-gray-400); |
There was a problem hiding this comment.
[quality] medium Priority
Issue: Inconsistent hover behavior for scrollbar thumb
Context: The --scrollbar-thumb-hover is set to the same value as --scrollbar-thumb in light theme, providing no visual feedback on hover.
Suggestion: Set --scrollbar-thumb-hover to a slightly darker shade like var(--color-gray-500) to provide clear hover feedback.
| scrollbar-width: thin; /* auto | thin | none */ | ||
| scrollbar-color: var(--scrollbar-thumb) var(--scrollbar-track); /* thumb track */ | ||
| /* Layout stability where supported */ | ||
| scrollbar-gutter: stable both-edges; /* ignored if unsupported */ |
There was a problem hiding this comment.
[performance] low Priority
Issue: Potential layout thrashing with scrollbar-gutter property
Context: The scrollbar-gutter: stable both-edges property can cause layout shifts on browsers where it's not supported or has inconsistent implementation.
Suggestion: Consider testing across target browsers or add fallback handling, especially for older Safari versions.
| - macOS/iOS show overlay scrollbars; thick tracks may appear slimmer there. | ||
| - Uses existing CSS variables (tokens) for colors and size. | ||
| ============================================================================ */ | ||
| @layer components { |
There was a problem hiding this comment.
[architecture] medium Priority
Issue: Custom CSS implementation instead of leveraging existing Tailwind utilities
Context: The project already uses a @Utility scrollbar-hide pattern at line 169, but the new scrollbar styling doesn't follow the same utility-first approach.
Suggestion: Consider implementing the custom scrollbar as a @Utility directive to maintain consistency with the existing scrollbar-hide utility and follow Tailwind's utility-first philosophy.
| listContainer: () => ({ | ||
| style: { maxHeight: listMaxHeight }, | ||
| class: 'overflow-y-auto scrollbar-hide' | ||
| class: 'overflow-y-auto custom-scrollbar' |
There was a problem hiding this comment.
[quality] low Priority
Issue: Good implementation pattern - clean class replacement
Context: The change from scrollbar-hide to custom-scrollbar maintains the same component architecture and follows Vue component best practices.
Suggestion: This is a clean implementation that properly separates concerns between styling and component logic.
| listContainer: () => ({ | ||
| style: { maxHeight: listMaxHeight }, | ||
| class: 'overflow-y-auto scrollbar-hide' | ||
| class: 'overflow-y-auto custom-scrollbar' |
There was a problem hiding this comment.
[architecture] medium Priority
Issue: Incomplete migration - other components still use scrollbar-hide
Context: A grep search reveals 8 other files still using scrollbar-hide class, creating inconsistency in scrollbar styling across the application.
Suggestion: Consider migrating all scrollbar-hide usages to custom-scrollbar for consistent styling, or provide clear guidance on when to use each approach.
There was a problem hiding this comment.
Comprehensive PR Review
This review is generated by Claude. It may not always be accurate, as with human reviewers. If you believe that any of the comments are invalid or incorrect, please state why for each. For others, please implement the changes in one way or another.
Review Summary
PR: [Style] Add custom scrollbar styling for SelectBox components (#5879)
Impact: 76 additions, 2 deletions across 3 files
Issue Distribution
- Critical: 0
- High: 1
- Medium: 4
- Low: 2
Category Breakdown
- Architecture: 3 issues
- Code Quality: 3 issues
- Performance: 1 issue
Key Findings
Architecture & Design
The implementation follows a reasonable approach for cross-browser scrollbar styling, but has some architectural inconsistencies. The PR introduces custom CSS components rather than following the existing Tailwind utility pattern used by scrollbar-hide. Additionally, the migration is incomplete - 8 other files still use scrollbar-hide, creating inconsistency across the application.
Code Quality Considerations
The main quality concern is the use of an undefined CSS color token (--color-white) which will cause fallbacks to browser defaults. The scrollbar hover behavior is also inconsistent in light theme where hover and normal states use the same color.
Performance Impact
The CSS includes scrollbar-gutter: stable both-edges which could cause layout shifts on browsers with inconsistent support. However, this is marked as low priority since it's ignored on unsupported browsers.
Integration Points
The Vue component changes are clean and maintain proper separation of concerns. The class replacement from scrollbar-hide to custom-scrollbar follows Vue best practices.
Positive Observations
- Comprehensive cross-browser support with both WebKit and Firefox implementations
- Well-documented CSS with clear usage instructions
- Clean Vue component implementation with proper class replacement
- Good use of CSS custom properties for theming
- Responsive design considerations for dark theme support
References
- Repository Architecture Guide
- ComfyUI Frontend Guidelines (CLAUDE.md) - emphasizes Tailwind utility-first approach
Next Steps
- Address the undefined color token issue (high priority)
- Consider completing the scrollbar migration across all components
- Evaluate whether to implement as Tailwind utility for consistency
- Test scrollbar behavior across target browsers
- Fix hover state inconsistency in light theme
This is a comprehensive automated review. The scrollbar implementation provides good user experience improvements, but addressing the color token and architectural consistency issues will improve maintainability.
Summary
Added custom scrollbar styling for SelectBox components (SingleSelect and MultiSelect) to improve visual consistency across the application.
Changes
.custom-scrollbarclass to SingleSelect and MultiSelect componentsImplementation Details
Browser Support
Screenshots
The custom scrollbar provides a more modern and consistent look across the application, especially in dropdown menus.
Testing
Before
After
┆Issue is synchronized with this Notion page by Unito