feat: add ColorSwatch components#2054
Closed
markjaniczak wants to merge 9 commits into
Closed
Conversation
commit: |
markjaniczak
changed the title
Member
|
Thanks for the PR @markjaniczak ! Will review this this week and perhaps introduce this new |
Contributor
Author
|
@zernonia any thoughts? I've got some time later this week to make any necessary changes. |
Member
|
@markjaniczak can you merge the latest v2, and allow me to push to your branch if possible? |
Member
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
|
✅ Actions performedReview triggered.
|
Contributor
📝 WalkthroughWalkthroughThis PR introduces two new color components—ColorSwatch and ColorSwatchPicker—with supporting utilities, comprehensive documentation, demo examples in CSS and Tailwind variants, and test coverage. ColorSwatch displays a single color; ColorSwatchPicker enables color selection via a picker interface composed of Root, Item, ItemSwatch, and ItemIndicator sub-components. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
packages/core/src/ColorSwatch/ColorSwatch.test.ts (1)
6-15: Tests cover basic accessibility and rendering.The test suite validates accessibility compliance and basic rendering. Consider expanding test coverage to include:
- Invalid color prop handling (the PR mentions dev warnings for invalid hex colors)
- Color slot functionality
- Custom label prop
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/ColorSwatch/ColorSwatch.test.ts` around lines 6 - 15, Add tests to ColorSwatch.test.ts that cover invalid hex color warnings, slot rendering, and custom label prop: add a test that mounts ColorSwatch with an invalid color prop (e.g., invalid hex) and asserts the dev warning is emitted (spy on console.warn or the library warning mechanism) referencing the ColorSwatch component and its color prop; add a test that mounts ColorSwatch with a color slot content and asserts the slot content is rendered (use mount(ColorSwatch, { slots: { color: '<...>' } }) and check wrapper.html()/text()); and add a test that passes a custom label prop (label) to ColorSwatch and asserts the label is displayed. Ensure tests reference ColorSwatch, the color prop, the label prop, and the color slot so they are easy to locate and maintain.packages/core/src/ColorSwatchPicker/ColorSwatchPickerRoot.vue (1)
42-43: Consider makingaria-labelconfigurable for localization.The
aria-label="color swatch options"is hardcoded in English. Per the PR discussion about localization considerations, this could be made configurable via props to support i18n.💡 Suggested approach
Add an optional
aria-labelprop toColorSwatchPickerRootProps:export interface ColorSwatchPickerRootProps extends Omit<ListboxRootProps, 'by'> { defaultValue?: string | string[] modelValue?: string | string[] + /** Accessible label for the color swatch picker. Defaults to "color swatch options". */ + label?: string }Then in the template:
<ListboxContent - aria-label="color swatch options" + :aria-label="label ?? 'color swatch options'" :as-child="asChild"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/ColorSwatchPicker/ColorSwatchPickerRoot.vue` around lines 42 - 43, The hardcoded aria-label on the ListboxContent ("color swatch options") prevents localization; add an optional prop (e.g. ariaLabel or listboxAriaLabel) to the ColorSwatchPickerRootProps and expose it on the component, defaulting to the existing English string, then bind that prop to the ListboxContent aria-label attribute in ColorSwatchPickerRoot.vue so consumers can provide localized text; update the prop type and default in the component definition and any TS interfaces referencing ColorSwatchPickerRootProps.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/content/meta/ColorSwatchPickerRoot.md`:
- Line 46: Update the user-facing description strings for the
ColorSwatchPickerRoot metadata: replace the incorrect phrase "binded with with
<code>v-model</code>" in the 'description' field with "bound with
<code>v-model</code>", and fix the other occurrence "the mouse leave" to a
grammatically correct phrase such as "when the mouse leaves" (both are in the
'description' keys for ColorSwatchPickerRoot). After editing those source
description strings, regenerate the component metadata so the updated
descriptions appear in the docs.
In `@packages/core/src/color/utils.ts`:
- Around line 33-52: The achromatic branch leaves l in 0–1 while the chromatic
branch scales s and l to 0–100, causing inconsistent thresholds in getColorName;
in the color conversion function (the block that assigns h, s, l for
achromatic/chromatic cases) multiply l by 100 in the achromatic branch (and
ensure s is 0) so both branches use 0–100 units, then update the comparisons
inside getColorName to use 0–100 scale (replace 0.8 with 80, 0.3 with 30, and
any other 0–1 thresholds accordingly) so all HSL logic uses the same scale.
- Around line 6-13: hexToRGB currently assumes hex is valid and uses parseInt
which yields NaN for bad input; update hexToRGB to validate the input string
(allow optional leading '#' and only 3 or 6 hex digits), normalize 3-digit
shorthand to 6-digit form, and throw a clear error (or return null) when the
format is invalid so downstream code doesn't get NaN values; locate the function
hexToRGB in packages/core/src/color/utils.ts and add the validation and
normalization before calling parseInt.
- Around line 110-118: The comparisons for saturation and lightness use 0–1
thresholds but s and l are 0–100; update the thresholds in the block that pushes
descriptors (the checks using variables s and l and calling
descriptors.push('vibrant'/'muted' and 'light'/'dark')) to use percentage values
(e.g., 80 and 30) instead of 0.8 and 0.3 so the vibrancy/muted and light/dark
logic correctly evaluates chromatic colors.
In `@packages/core/src/ColorSwatch/ColorSwatch.vue`:
- Around line 24-38: The computed label currently returns undefined when
getColorName(props.color) throws and no props.label is provided, leaving
aria-label empty; update the label computation in the ColorSwatch component (the
computed named label used for aria-label) to fall back to props.color (the raw
hex value) as a last resort when getColorName fails or is unavailable, and apply
the same fallback logic to the other similar computed/usage block noted around
lines 54-58 so the component always provides an accessible name.
In `@packages/core/src/ColorSwatch/story/_ColorSwatch.vue`:
- Line 15: The template in the _ColorSwatch.vue component contains a duplicated
self-closing sequence "/> />" (duplicate closing tag) that causes a parser
error; open the template in
packages/core/src/ColorSwatch/story/_ColorSwatch.vue, locate the line with the
duplicate "/> />" in the _ColorSwatch component's template, and remove the extra
"/>" so the element is correctly self-closed only once (i.e., change "/> />" to
a single "/>").
---
Nitpick comments:
In `@packages/core/src/ColorSwatch/ColorSwatch.test.ts`:
- Around line 6-15: Add tests to ColorSwatch.test.ts that cover invalid hex
color warnings, slot rendering, and custom label prop: add a test that mounts
ColorSwatch with an invalid color prop (e.g., invalid hex) and asserts the dev
warning is emitted (spy on console.warn or the library warning mechanism)
referencing the ColorSwatch component and its color prop; add a test that mounts
ColorSwatch with a color slot content and asserts the slot content is rendered
(use mount(ColorSwatch, { slots: { color: '<...>' } }) and check
wrapper.html()/text()); and add a test that passes a custom label prop (label)
to ColorSwatch and asserts the label is displayed. Ensure tests reference
ColorSwatch, the color prop, the label prop, and the color slot so they are easy
to locate and maintain.
In `@packages/core/src/ColorSwatchPicker/ColorSwatchPickerRoot.vue`:
- Around line 42-43: The hardcoded aria-label on the ListboxContent ("color
swatch options") prevents localization; add an optional prop (e.g. ariaLabel or
listboxAriaLabel) to the ColorSwatchPickerRootProps and expose it on the
component, defaulting to the existing English string, then bind that prop to the
ListboxContent aria-label attribute in ColorSwatchPickerRoot.vue so consumers
can provide localized text; update the prop type and default in the component
definition and any TS interfaces referencing ColorSwatchPickerRootProps.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
docs/.vitepress/config.tsdocs/components/demo/ColorSwatch/css/index.vuedocs/components/demo/ColorSwatch/css/styles.cssdocs/components/demo/ColorSwatch/tailwind/index.vuedocs/components/demo/ColorSwatch/tailwind/tailwind.config.jsdocs/components/demo/ColorSwatchPicker/css/index.vuedocs/components/demo/ColorSwatchPicker/css/styles.cssdocs/components/demo/ColorSwatchPicker/tailwind/index.vuedocs/components/demo/ColorSwatchPicker/tailwind/tailwind.config.jsdocs/content/docs/components/color-swatch-picker.mddocs/content/docs/components/color-swatch.mddocs/content/meta/ColorSwatch.mddocs/content/meta/ColorSwatchPickerItem.mddocs/content/meta/ColorSwatchPickerItemIndicator.mddocs/content/meta/ColorSwatchPickerItemSwatch.mddocs/content/meta/ColorSwatchPickerRoot.mdpackages/core/constant/components.tspackages/core/src/ColorSwatch/ColorSwatch.test.tspackages/core/src/ColorSwatch/ColorSwatch.vuepackages/core/src/ColorSwatch/index.tspackages/core/src/ColorSwatch/story/_ColorSwatch.vuepackages/core/src/ColorSwatchPicker/ColorSwatchPicker.test.tspackages/core/src/ColorSwatchPicker/ColorSwatchPickerItem.vuepackages/core/src/ColorSwatchPicker/ColorSwatchPickerItemIndicator.vuepackages/core/src/ColorSwatchPicker/ColorSwatchPickerItemSwatch.vuepackages/core/src/ColorSwatchPicker/ColorSwatchPickerRoot.vuepackages/core/src/ColorSwatchPicker/index.tspackages/core/src/ColorSwatchPicker/story/_ColorSwatchPicker.vuepackages/core/src/color/index.tspackages/core/src/color/utils.tspackages/core/src/index.ts
| }, | ||
| { | ||
| 'name': 'modelValue', | ||
| 'description': '<p>The controlled value of the listbox. Can be binded with with <code>v-model</code>.</p>\n', |
Contributor
There was a problem hiding this comment.
Fix user-facing wording in generated API descriptions.
Line 46 has “binded with with”, and Line 96 has “the mouse leave”. Please fix the source description text and regenerate this metadata so docs read correctly.
Also applies to: 96-96
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/content/meta/ColorSwatchPickerRoot.md` at line 46, Update the
user-facing description strings for the ColorSwatchPickerRoot metadata: replace
the incorrect phrase "binded with with <code>v-model</code>" in the
'description' field with "bound with <code>v-model</code>", and fix the other
occurrence "the mouse leave" to a grammatically correct phrase such as "when the
mouse leaves" (both are in the 'description' keys for ColorSwatchPickerRoot).
After editing those source description strings, regenerate the component
metadata so the updated descriptions appear in the docs.
Comment on lines
+6
to
+13
| export function hexToRGB(hex: string): { r: number, g: number, b: number } { | ||
| hex = hex.replace(/^#/, '') | ||
| const bigint = parseInt(hex, 16) | ||
| const r = (bigint >> 16) & 255 | ||
| const g = (bigint >> 8) & 255 | ||
| const b = bigint & 255 | ||
| return { r, g, b } | ||
| } |
Contributor
There was a problem hiding this comment.
Missing input validation for hex string.
hexToRGB doesn't validate the input format. Invalid hex strings (e.g., "invalid", "#GGG") will silently produce incorrect RGB values via parseInt(..., 16) returning NaN for invalid characters.
🛡️ Suggested validation
export function hexToRGB(hex: string): { r: number, g: number, b: number } {
hex = hex.replace(/^#/, '')
+ if (!/^[0-9A-Fa-f]{6}$/.test(hex) && !/^[0-9A-Fa-f]{3}$/.test(hex)) {
+ throw new Error(`Invalid hex color: ${hex}`)
+ }
+ // Handle shorthand hex (e.g., `#FFF` -> `#FFFFFF`)
+ if (hex.length === 3) {
+ hex = hex.split('').map(c => c + c).join('')
+ }
const bigint = parseInt(hex, 16)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/color/utils.ts` around lines 6 - 13, hexToRGB currently
assumes hex is valid and uses parseInt which yields NaN for bad input; update
hexToRGB to validate the input string (allow optional leading '#' and only 3 or
6 hex digits), normalize 3-digit shorthand to 6-digit form, and throw a clear
error (or return null) when the format is invalid so downstream code doesn't get
NaN values; locate the function hexToRGB in packages/core/src/color/utils.ts and
add the validation and normalization before calling parseInt.
Comment on lines
+33
to
+52
| if (max === min) { | ||
| h = s = 0 // achromatic | ||
| } | ||
| else { | ||
| const d = max - min | ||
| s = l > 0.5 ? d / (2 - max - min) : d / (max + min) | ||
| if (max === r) { | ||
| h = (g - b) / d + (g < b ? 6 : 0) | ||
| } | ||
| else if (max === g) { | ||
| h = (b - r) / d + 2 | ||
| } | ||
| else { | ||
| h = (r - g) / d + 4 | ||
| } | ||
| h /= 6 | ||
| h *= 360 | ||
| s *= 100 | ||
| l *= 100 | ||
| } |
Contributor
There was a problem hiding this comment.
Scale inconsistency between achromatic and chromatic HSL values.
In the achromatic branch (lines 33-35), l remains in the 0–1 range, but in the chromatic branch (lines 48-51), s and l are scaled to 0–100. This causes getColorName to apply inconsistent thresholds—lines 110-118 compare against 0.8 and 0.3, which work for 0–1 scale but not for 0–100.
🐛 Proposed fix: Scale l in achromatic case too
if (max === min) {
h = s = 0 // achromatic
+ l *= 100
}
else {Then update getColorName thresholds in lines 66-79 accordingly (multiply by 100):
- if (s < 0.1) {
- if (l < 0.1)
+ if (s < 10) {
+ if (l < 10)
return 'black'
- if (l > 0.95)
+ if (l > 95)
return 'white'
// ... similar updates for other thresholds🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/color/utils.ts` around lines 33 - 52, The achromatic branch
leaves l in 0–1 while the chromatic branch scales s and l to 0–100, causing
inconsistent thresholds in getColorName; in the color conversion function (the
block that assigns h, s, l for achromatic/chromatic cases) multiply l by 100 in
the achromatic branch (and ensure s is 0) so both branches use 0–100 units, then
update the comparisons inside getColorName to use 0–100 scale (replace 0.8 with
80, 0.3 with 30, and any other 0–1 thresholds accordingly) so all HSL logic uses
the same scale.
Comment on lines
+110
to
+118
| if (s > 0.8) | ||
| descriptors.push('vibrant') | ||
| else if (s < 0.3) | ||
| descriptors.push('muted') | ||
|
|
||
| if (l > 0.8) | ||
| descriptors.push('light') | ||
| else if (l < 0.3) | ||
| descriptors.push('dark') |
Contributor
There was a problem hiding this comment.
Threshold values don't match the 0–100 scale.
As noted above, s and l are in the 0–100 range for chromatic colors, but these comparisons use 0–1 scale thresholds.
🐛 Fix thresholds to use percentage scale
const descriptors = []
- if (s > 0.8)
+ if (s > 80)
descriptors.push('vibrant')
- else if (s < 0.3)
+ else if (s < 30)
descriptors.push('muted')
- if (l > 0.8)
+ if (l > 80)
descriptors.push('light')
- else if (l < 0.3)
+ else if (l < 30)
descriptors.push('dark')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (s > 0.8) | |
| descriptors.push('vibrant') | |
| else if (s < 0.3) | |
| descriptors.push('muted') | |
| if (l > 0.8) | |
| descriptors.push('light') | |
| else if (l < 0.3) | |
| descriptors.push('dark') | |
| if (s > 80) | |
| descriptors.push('vibrant') | |
| else if (s < 30) | |
| descriptors.push('muted') | |
| if (l > 80) | |
| descriptors.push('light') | |
| else if (l < 30) | |
| descriptors.push('dark') |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/color/utils.ts` around lines 110 - 118, The comparisons for
saturation and lightness use 0–1 thresholds but s and l are 0–100; update the
thresholds in the block that pushes descriptors (the checks using variables s
and l and calling descriptors.push('vibrant'/'muted' and 'light'/'dark')) to use
percentage values (e.g., 80 and 30) instead of 0.8 and 0.3 so the vibrancy/muted
and light/dark logic correctly evaluates chromatic colors.
Comment on lines
+24
to
+38
| const label = computed(() => { | ||
| if (props.label) | ||
| return props.label | ||
|
|
||
| try { | ||
| return `${getColorName(props.color)}` | ||
| } | ||
| catch { | ||
| if (import.meta.env.DEV) { | ||
| console.warn(`WARNING: Unable to resolve color "${props.color}" to a name. | ||
| Please check that the color provided is a valid hex color or provide a label.`) | ||
| } | ||
| return undefined | ||
| } | ||
| }) |
Contributor
There was a problem hiding this comment.
Consider fallback for aria-label when color resolution fails.
When getColorName throws and no label prop is provided, aria-label becomes undefined. This leaves the component without an accessible name, which may impact screen reader users. Consider falling back to the raw hex value as a last resort.
💡 Suggested improvement
const label = computed(() => {
if (props.label)
return props.label
try {
return `${getColorName(props.color)}`
}
catch {
if (import.meta.env.DEV) {
console.warn(`WARNING: Unable to resolve color "${props.color}" to a name.
Please check that the color provided is a valid hex color or provide a label.`)
}
- return undefined
+ return props.color || undefined
}
})Also applies to: 54-58
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/ColorSwatch/ColorSwatch.vue` around lines 24 - 38, The
computed label currently returns undefined when getColorName(props.color) throws
and no props.label is provided, leaving aria-label empty; update the label
computation in the ColorSwatch component (the computed named label used for
aria-label) to fall back to props.color (the raw hex value) as a last resort
when getColorName fails or is unavailable, and apply the same fallback logic to
the other similar computed/usage block noted around lines 54-58 so the component
always provides an accessible name.
| v-bind="props" | ||
| :color="color" | ||
| class="w-8 h-8 rounded ring-1 ring-inset ring-black/15 dark:ring-white/15" | ||
| /> /> |
Contributor
There was a problem hiding this comment.
Duplicate closing tag syntax error.
There's a duplicate /> on line 15 which will cause a template parsing error.
🐛 Proposed fix
:color="color"
class="w-8 h-8 rounded ring-1 ring-inset ring-black/15 dark:ring-white/15"
- /> />
+ />📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /> /> | |
| :color="color" | |
| class="w-8 h-8 rounded ring-1 ring-inset ring-black/15 dark:ring-white/15" | |
| /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/ColorSwatch/story/_ColorSwatch.vue` at line 15, The
template in the _ColorSwatch.vue component contains a duplicated self-closing
sequence "/> />" (duplicate closing tag) that causes a parser error; open the
template in packages/core/src/ColorSwatch/story/_ColorSwatch.vue, locate the
line with the duplicate "/> />" in the _ColorSwatch component's template, and
remove the extra "/>" so the element is correctly self-closed only once (i.e.,
change "/> />" to a single "/>").
Contributor
📝 WalkthroughWalkthroughIntroduces ColorSwatch and ColorSwatchPicker components with accompanying color utility functions. Adds comprehensive documentation, demo examples, tests, and navigation updates to expose the new components within the package's public API and documentation site. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
docs/content/meta/ColorSwatchPickerRoot.md (1)
46-46:⚠️ Potential issue | 🟡 MinorFix grammar in generated API descriptions and regenerate metadata.
Line 46 still says “binded with with”, and Line 96 says “mouse leave”. Please correct both source descriptions, then regenerate this auto-generated file so docs render cleanly.
Also applies to: 96-96
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/meta/ColorSwatchPickerRoot.md` at line 46, The generated doc for ColorSwatchPickerRoot contains two grammar errors: change the prop description that currently reads "Can be binded with with <code>v-model</code>." to "Can be bound with <code>v-model</code>." and correct the text that reads "mouse leave" to the intended term (e.g., "mouseleave" or "mouse leave event" depending on the source). Update the original source descriptions used to generate ColorSwatchPickerRoot metadata (the strings that produce the 'description' entries) and then regenerate the auto-generated metadata so the docs reflect the corrected phrasing.
🧹 Nitpick comments (4)
packages/core/src/ColorSwatchPicker/ColorSwatchPickerRoot.vue (2)
28-31: Theas falsetype assertion is a known VueUse idiom but warrants a brief comment.The cast
(props.modelValue === undefined) as falseworks correctly—enabling passive mode when uncontrolled—but may confuse future readers. A short inline comment clarifying this VueUse pattern would improve maintainability.📝 Suggested comment
const modelValue = useVModel(props, 'modelValue', emits, { defaultValue: props.defaultValue, + // Cast required for VueUse's passive option type; enables passive sync when modelValue is undefined passive: (props.modelValue === undefined) as false, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/ColorSwatchPicker/ColorSwatchPickerRoot.vue` around lines 28 - 31, Add a short inline comment next to the passive flag in the useVModel call to clarify the VueUse idiom: explain that (props.modelValue === undefined) is intentionally cast to false to enable passive (uncontrolled) mode when no modelValue prop is provided; update the line with the modelValue = useVModel(...) call (referencing useVModel, modelValue, and props.modelValue) so the intent is clear to future readers.
42-44: Consider making the aria-label configurable for i18n support.The hardcoded
aria-label="color swatch options"may need localization support, as noted in the PR discussion about English-only color descriptions. Consider exposing this as a prop or integrating with the ConfigProvider localization mechanism.💡 Suggested approach
export interface ColorSwatchPickerRootProps extends Omit<ListboxRootProps, 'by'> { defaultValue?: string | string[] modelValue?: string | string[] + /** + * Accessible label for the color swatch picker. + * `@default` 'color swatch options' + */ + ariaLabel?: string }Then in the template:
<ListboxContent - aria-label="color swatch options" + :aria-label="ariaLabel" :as-child="asChild" :as="as" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/ColorSwatchPicker/ColorSwatchPickerRoot.vue` around lines 42 - 44, Make the ListboxContent aria-label configurable instead of hardcoding "color swatch options": add a prop on ColorSwatchPickerRoot.vue (e.g., ariaLabel or listboxAriaLabel) and pass it into the <ListboxContent :aria-label="...">; default the prop to the existing English string and wire it to the app ConfigProvider/localization if available so consumers can override it for i18n. Locate the ListboxContent usage and the ColorSwatchPickerRoot component definition to add the prop, default, and pass-through.packages/core/src/ColorSwatch/ColorSwatch.test.ts (1)
6-15: Consider expanding test coverage.The current tests verify accessibility and basic rendering, which is a good start. Consider adding tests for:
- Custom
labelprop overriding the derived color name- Handling of invalid color values (verifying graceful degradation)
- Slot content rendering
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/ColorSwatch/ColorSwatch.test.ts` around lines 6 - 15, Add unit tests in ColorSwatch.test.ts that cover three scenarios missing from current specs: (1) pass a custom label prop to the ColorSwatch mount and assert the rendered label overrides the derived color name (use mount(ColorSwatch, { props: { label: '...' } }) and assert wrapper.text()/wrapper.find(...).text()), (2) mount ColorSwatch with invalid color values (e.g., malformed hex/string via props used by the component) and assert the component degrades gracefully (no crash, shows fallback UI or empty swatch and an accessible aria/state), and (3) verify slot rendering by mounting ColorSwatch with default/named slot content and asserting that wrapper.html()/wrapper.find(...).exists() contains the slot markup; ensure each new test still runs axe(wrapper.element) for accessibility where appropriate.docs/components/demo/ColorSwatchPicker/tailwind/index.vue (1)
26-26: Add an accessible name to the picker root in the demo.Giving the listbox an explicit label improves accessibility in example code.
Suggested diff
- <ColorSwatchPickerRoot class="flex gap-2"> + <ColorSwatchPickerRoot aria-label="Select a color" class="flex gap-2">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/components/demo/ColorSwatchPicker/tailwind/index.vue` at line 26, The demo's ColorSwatchPickerRoot element lacks an accessible name; update the ColorSwatchPickerRoot usage to provide one (e.g., add aria-label or aria-labelledby pointing to a visible label element) so the listbox has an explicit label for screen readers; target the ColorSwatchPickerRoot component in the demo and either add aria-label="Color picker" (or similar) directly or render a <label> with an id and set aria-labelledby on ColorSwatchPickerRoot to that id.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/ColorSwatch/ColorSwatch.test.ts`:
- Line 4: The test imports the story component `_ColorSwatch.vue` which contains
a duplicate closing tag causing a syntax error and failing tests; open the
`_ColorSwatch.vue` story used by the ColorSwatch test (referenced by import
ColorSwatch from './story/_ColorSwatch.vue') and remove the duplicate/extra
closing tag so the template is well-formed, then run the tests to confirm the
syntax error is resolved.
In `@packages/core/src/ColorSwatchPicker/ColorSwatchPicker.test.ts`:
- Around line 12-17: Add an explicit cleanup step to unmount the mounted
component after each test to avoid DOM/listener leaks: in the test file where
you use mount(ColorSwatchPicker, { attachTo: document.body }) and set up in
beforeEach, add an afterEach that checks the wrapper and calls wrapper.unmount()
(or wrapper.destroy() if using an older Vue Test Utils API) and then nulls the
wrapper variable so the DOM and event listeners are removed between tests.
---
Duplicate comments:
In `@docs/content/meta/ColorSwatchPickerRoot.md`:
- Line 46: The generated doc for ColorSwatchPickerRoot contains two grammar
errors: change the prop description that currently reads "Can be binded with
with <code>v-model</code>." to "Can be bound with <code>v-model</code>." and
correct the text that reads "mouse leave" to the intended term (e.g.,
"mouseleave" or "mouse leave event" depending on the source). Update the
original source descriptions used to generate ColorSwatchPickerRoot metadata
(the strings that produce the 'description' entries) and then regenerate the
auto-generated metadata so the docs reflect the corrected phrasing.
---
Nitpick comments:
In `@docs/components/demo/ColorSwatchPicker/tailwind/index.vue`:
- Line 26: The demo's ColorSwatchPickerRoot element lacks an accessible name;
update the ColorSwatchPickerRoot usage to provide one (e.g., add aria-label or
aria-labelledby pointing to a visible label element) so the listbox has an
explicit label for screen readers; target the ColorSwatchPickerRoot component in
the demo and either add aria-label="Color picker" (or similar) directly or
render a <label> with an id and set aria-labelledby on ColorSwatchPickerRoot to
that id.
In `@packages/core/src/ColorSwatch/ColorSwatch.test.ts`:
- Around line 6-15: Add unit tests in ColorSwatch.test.ts that cover three
scenarios missing from current specs: (1) pass a custom label prop to the
ColorSwatch mount and assert the rendered label overrides the derived color name
(use mount(ColorSwatch, { props: { label: '...' } }) and assert
wrapper.text()/wrapper.find(...).text()), (2) mount ColorSwatch with invalid
color values (e.g., malformed hex/string via props used by the component) and
assert the component degrades gracefully (no crash, shows fallback UI or empty
swatch and an accessible aria/state), and (3) verify slot rendering by mounting
ColorSwatch with default/named slot content and asserting that
wrapper.html()/wrapper.find(...).exists() contains the slot markup; ensure each
new test still runs axe(wrapper.element) for accessibility where appropriate.
In `@packages/core/src/ColorSwatchPicker/ColorSwatchPickerRoot.vue`:
- Around line 28-31: Add a short inline comment next to the passive flag in the
useVModel call to clarify the VueUse idiom: explain that (props.modelValue ===
undefined) is intentionally cast to false to enable passive (uncontrolled) mode
when no modelValue prop is provided; update the line with the modelValue =
useVModel(...) call (referencing useVModel, modelValue, and props.modelValue) so
the intent is clear to future readers.
- Around line 42-44: Make the ListboxContent aria-label configurable instead of
hardcoding "color swatch options": add a prop on ColorSwatchPickerRoot.vue
(e.g., ariaLabel or listboxAriaLabel) and pass it into the <ListboxContent
:aria-label="...">; default the prop to the existing English string and wire it
to the app ConfigProvider/localization if available so consumers can override it
for i18n. Locate the ListboxContent usage and the ColorSwatchPickerRoot
component definition to add the prop, default, and pass-through.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
docs/.vitepress/config.tsdocs/components/demo/ColorSwatch/css/index.vuedocs/components/demo/ColorSwatch/css/styles.cssdocs/components/demo/ColorSwatch/tailwind/index.vuedocs/components/demo/ColorSwatch/tailwind/tailwind.config.jsdocs/components/demo/ColorSwatchPicker/css/index.vuedocs/components/demo/ColorSwatchPicker/css/styles.cssdocs/components/demo/ColorSwatchPicker/tailwind/index.vuedocs/components/demo/ColorSwatchPicker/tailwind/tailwind.config.jsdocs/content/docs/components/color-swatch-picker.mddocs/content/docs/components/color-swatch.mddocs/content/meta/ColorSwatch.mddocs/content/meta/ColorSwatchPickerItem.mddocs/content/meta/ColorSwatchPickerItemIndicator.mddocs/content/meta/ColorSwatchPickerItemSwatch.mddocs/content/meta/ColorSwatchPickerRoot.mdpackages/core/constant/components.tspackages/core/src/ColorSwatch/ColorSwatch.test.tspackages/core/src/ColorSwatch/ColorSwatch.vuepackages/core/src/ColorSwatch/index.tspackages/core/src/ColorSwatch/story/_ColorSwatch.vuepackages/core/src/ColorSwatchPicker/ColorSwatchPicker.test.tspackages/core/src/ColorSwatchPicker/ColorSwatchPickerItem.vuepackages/core/src/ColorSwatchPicker/ColorSwatchPickerItemIndicator.vuepackages/core/src/ColorSwatchPicker/ColorSwatchPickerItemSwatch.vuepackages/core/src/ColorSwatchPicker/ColorSwatchPickerRoot.vuepackages/core/src/ColorSwatchPicker/index.tspackages/core/src/ColorSwatchPicker/story/_ColorSwatchPicker.vuepackages/core/src/color/index.tspackages/core/src/color/utils.tspackages/core/src/index.ts
| import { mount } from '@vue/test-utils' | ||
| import { describe, expect, it } from 'vitest' | ||
| import { axe } from 'vitest-axe' | ||
| import ColorSwatch from './story/_ColorSwatch.vue' |
Contributor
There was a problem hiding this comment.
Tests depend on story component with syntax error.
The test imports _ColorSwatch.vue which currently has a duplicate closing tag syntax error. This will cause test failures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/ColorSwatch/ColorSwatch.test.ts` at line 4, The test
imports the story component `_ColorSwatch.vue` which contains a duplicate
closing tag causing a syntax error and failing tests; open the
`_ColorSwatch.vue` story used by the ColorSwatch test (referenced by import
ColorSwatch from './story/_ColorSwatch.vue') and remove the duplicate/extra
closing tag so the template is well-formed, then run the tests to confirm the
syntax error is resolved.
Comment on lines
+12
to
+17
| beforeEach(() => { | ||
| document.body.innerHTML = '' | ||
| wrapper = mount(ColorSwatchPicker, { attachTo: document.body }) | ||
| content = wrapper.find('[role=listbox]') | ||
| items = wrapper.findAll('[role=option]') | ||
| }) |
Contributor
There was a problem hiding this comment.
Add explicit wrapper cleanup between tests.
Because the component is mounted with attachTo: document.body, not unmounting can leak DOM/listeners across cases and introduce flaky tests.
💡 Suggested fix
-import { beforeEach, describe, expect, it } from 'vitest'
+import { afterEach, beforeEach, describe, expect, it } from 'vitest'
@@
beforeEach(() => {
document.body.innerHTML = ''
wrapper = mount(ColorSwatchPicker, { attachTo: document.body })
content = wrapper.find('[role=listbox]')
items = wrapper.findAll('[role=option]')
})
+
+ afterEach(() => {
+ wrapper.unmount()
+ })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| beforeEach(() => { | |
| document.body.innerHTML = '' | |
| wrapper = mount(ColorSwatchPicker, { attachTo: document.body }) | |
| content = wrapper.find('[role=listbox]') | |
| items = wrapper.findAll('[role=option]') | |
| }) | |
| import { afterEach, beforeEach, describe, expect, it } from 'vitest' | |
| beforeEach(() => { | |
| document.body.innerHTML = '' | |
| wrapper = mount(ColorSwatchPicker, { attachTo: document.body }) | |
| content = wrapper.find('[role=listbox]') | |
| items = wrapper.findAll('[role=option]') | |
| }) | |
| afterEach(() => { | |
| wrapper.unmount() | |
| }) |
🧰 Tools
🪛 ast-grep (0.41.0)
[warning] 12-12: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: document.body.innerHTML = ''
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/ColorSwatchPicker/ColorSwatchPicker.test.ts` around lines
12 - 17, Add an explicit cleanup step to unmount the mounted component after
each test to avoid DOM/listener leaks: in the test file where you use
mount(ColorSwatchPicker, { attachTo: document.body }) and set up in beforeEach,
add an afterEach that checks the wrapper and calls wrapper.unmount() (or
wrapper.destroy() if using an older Vue Test Utils API) and then nulls the
wrapper variable so the DOM and event listeners are removed between tests.
Member
|
Thanks for the PR @markjaniczak ! Because I dont have the permission to push to this remote, I've created another branch here #2478, will close this 😁 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Working towards adding a variety of color controls as described in #501.
The motivation for me is that our company is using Reka UI and we're in need of a swatch controls only, so I may be slower in implementing other types of color controls.
I've been using React Aria as inspiration so far.
Some things to consider:
ColorSwatchjust logs a warning in development if the color string provided is a not a valid colour hex string. Should this behaviour be stricter to enforce valid colours? Or, perhaps accept an object instead to reduce errors like{ r: number; g: number; b: number; }ConfigProvider?Summary by CodeRabbit
Release Notes
New Features
Documentation