Add support for 'scrollbarColor' theme property#98
Add support for 'scrollbarColor' theme property#98kaelansmith wants to merge 1 commit intoadoxography:mainfrom
Conversation
|
Hi @adoxography, just following up about this PR -- any concerns/questions? |
|
Hi again @adoxography, sorry to bug you -- I assume you're busy. I would really love to use this feature in prod soon, and would prefer not to have to fork the package just for this simple change; any chance you can review and, assuming all looks good, merge/release a new package version? |
|
@adoxography poking you once more (sorry)! |
|
Hi @adoxography, checking in once more -- just looking for a response of any type before I move ahead with forking |
adoxography
left a comment
adoxography
left a comment
There was a problem hiding this comment.
Overall, I'm fine with the concept, but I'd want a little more out of this before I'd consider merging it:
- Defining custom scrollbar theme properties shouldn't prevent you from using global colours
- CSS config support
- Tests to prove that this works.
| */ | ||
| const addColorUtilities = ({ matchUtilities, theme }) => { | ||
| const themeColors = flattenColorPalette(theme('colors')); | ||
| const themeColors = theme('scrollbarColor') ?? theme('colors'); |
There was a problem hiding this comment.
This shadows your global colors definitions. If you've defined scrollbarColor in your config, this change would prevent you from using any colours for scrollbars that aren't a scrollbarColor.
| */ | ||
| const addColorUtilities = ({ matchUtilities, theme }) => { | ||
| const themeColors = flattenColorPalette(theme('colors')); | ||
| const themeColors = theme('scrollbarColor') ?? theme('colors'); |
There was a problem hiding this comment.
The key scrollbarColor works with legacy JavaScript configs, but it won't play nicely with the CSS-based config introduced in Tailwind v4. As pointed out in #109, it's common to configure prettier to lowercase all of your CSS custom properties.
The new (undocumented) pattern Tailwind 4 is following for this is kebab-case (e.g. --border-color-neutral); so in this case, colours defined as --scrollbar-color-____ should also be recognized.
See tailwindlabs/tailwindcss#16512, tailwindlabs/tailwindcss#15757
Happily, it looks like theme('scrollbar-color') does what you hope it will - so there isn't any technical limitation here.
| const themeColors = theme('scrollbarColor') ?? theme('colors'); | ||
| const colors = Object.fromEntries( | ||
| Object.entries(themeColors).map(([k, v]) => [k, toColorValue(v)]) | ||
| Object.entries(flattenColorPalette(themeColors)).map(([k, v]) => [k, toColorValue(v)]) |
There was a problem hiding this comment.
Nit: this line becomes (even more) cognitively heavy by moving flattenColorPalette into it. I'd generally prefer to assign to a different variable and leave this line alone.
2 participants
Currently the plugin only pulls colors from the global "colors" theme property; it would be nice to be able to specify scrollbar-specific colors via its own theme color property, labelled
scrollbarColor. This is the norm for all other color-related utilities; eg. you can specify utility-specific color palettes undertheme.textColor,theme.borderColor, etc... so this PR adds support for this. If you don't specify a scrollbar-specific color palette, it falls back to the global "colors" -- so this PR doesn't introduce any breaking changes.