fix: make Color Picker Widget coerce to HEX with hashtag regardless of format/value in the UI (#5472)

* fix color picker value prefix and add component tests

* test(widgets): make color text assertion specific in WidgetColorPicker.test per review (DrJKL)

* test(widgets): use expect.soft for valid hex colors loop (suggestion by DrJKL)

* test(widgets): normalize color display to single leading # to address review question (AustinMroz)

* feat(widgets): normalize color widget values to #hex across inputs (hex/rgb/hsb); always emit with leading # using colorUtil conversions

* test(widgets): use data-testid selector for color text instead of generic span; add data-testid to component span for robustness

* support hsb|rgb|hex and coerce to hex with hashtag internally

refactor(widgets,utils): format-driven color normalization to lowercase #hex without casts; add typed toHexFromFormat and guards; simplify WidgetColorPicker state and types\n\n- utils: add ColorFormat, HSB/HSV types, isColorFormat/isHSBObject/isHSVObject, toHexFromFormat; reuse parseToRgb/hsbToRgb/rgbToHex\n- widgets: emit normalized #hex, display derived via toHexFromFormat, keep picker native v-model; typed widget options {format?}\n- tests: consolidate colorUtil tests into tests-ui/tests/colorUtil.test.ts; keep conversion + adjustColor suites; selectors robust\n- docs: add PR-5472-change-summary.md explaining changes\n\nAll type checks pass; ready for your final review before push.

refactor(widgets,utils): format-driven color normalization to lowercase #hex without casts; add typed toHexFromFormat and guards; simplify WidgetColorPicker state and types\n\n- utils: add ColorFormat, HSB/HSV types, isColorFormat/isHSBObject/isHSVObject, toHexFromFormat; reuse parseToRgb/hsbToRgb/rgbToHex\n- widgets: emit normalized #hex, display derived via toHexFromFormat, keep picker native v-model; typed widget options {format?}\n- tests: consolidate colorUtil tests into tests-ui/tests/colorUtil.test.ts; keep conversion + adjustColor suites; selectors robust\n- docs: add PR-5472-change-summary.md explaining changes\n\nAll type checks pass; ready for your final review before push.

chore: untrack PR-5472-change-summary.md and ignore locally (keep file on disk)

* fix(utils): use floor in hsbToRgb to match expected hex (#7f0000) for 50% brightness rounding behavior

* test(widgets): restore invalid-format fallback test and use data-testid selector in hex loop; chore: revert .gitignore change (remove PR-5472-change-summary.md entry)

* chore: restore .gitignore to match main (remove local note/comment)

* [refactor] improve color parsing in ColorPicker widget - addresses review feedback

- Use fancy color parsing for initial value normalization per @DrJKL's suggestion
- Simplify onPickerUpdate to trust configured format per @AustinMroz's feedback
- Remove redundant type checking and format guessing logic

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>

* [refactor] simplify color parsing - remove unnecessary helper function

- Remove normalizeColorValue helper and inline null checks
- Remove verbose comments
- Keep the same functionality with cleaner code

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>

* remove unused exports

---------

Co-authored-by: Claude <noreply@anthropic.com>
This commit is contained in:
Christian Byrne
2025-09-13 19:58:26 -07:00
committed by GitHub
parent 77ee20597f
commit 4604bbd669
4 changed files with 557 additions and 15 deletions

View File

@@ -0,0 +1,304 @@
import { mount } from '@vue/test-utils'
import ColorPicker from 'primevue/colorpicker'
import type { ColorPickerProps } from 'primevue/colorpicker'
import PrimeVue from 'primevue/config'
import { describe, expect, it } from 'vitest'
import type { SimplifiedWidget } from '@/types/simplifiedWidget'
import WidgetColorPicker from './WidgetColorPicker.vue'
import WidgetLayoutField from './layout/WidgetLayoutField.vue'
describe('WidgetColorPicker Value Binding', () => {
const createMockWidget = (
value: string = '#000000',
options: Partial<ColorPickerProps> = {},
callback?: (value: string) => void
): SimplifiedWidget<string> => ({
name: 'test_color_picker',
type: 'color',
value,
options,
callback
})
const mountComponent = (
widget: SimplifiedWidget<string>,
modelValue: string,
readonly = false
) => {
return mount(WidgetColorPicker, {
global: {
plugins: [PrimeVue],
components: {
ColorPicker,
WidgetLayoutField
}
},
props: {
widget,
modelValue,
readonly
}
})
}
const setColorPickerValue = async (
wrapper: ReturnType<typeof mount>,
value: unknown
) => {
const colorPicker = wrapper.findComponent({ name: 'ColorPicker' })
await colorPicker.setValue(value)
return wrapper.emitted('update:modelValue')
}
describe('Vue Event Emission', () => {
it('emits Vue event when color changes', async () => {
const widget = createMockWidget('#ff0000')
const wrapper = mountComponent(widget, '#ff0000')
const emitted = await setColorPickerValue(wrapper, '#00ff00')
expect(emitted).toBeDefined()
expect(emitted![0]).toContain('#00ff00')
})
it('handles different color formats', async () => {
const widget = createMockWidget('#ffffff')
const wrapper = mountComponent(widget, '#ffffff')
const emitted = await setColorPickerValue(wrapper, '#123abc')
expect(emitted).toBeDefined()
expect(emitted![0]).toContain('#123abc')
})
it('handles missing callback gracefully', async () => {
const widget = createMockWidget('#000000', {}, undefined)
const wrapper = mountComponent(widget, '#000000')
const emitted = await setColorPickerValue(wrapper, '#ff00ff')
// Should still emit Vue event
expect(emitted).toBeDefined()
expect(emitted![0]).toContain('#ff00ff')
})
it('normalizes bare hex without # to #hex on emit', async () => {
const widget = createMockWidget('ff0000')
const wrapper = mountComponent(widget, 'ff0000')
const emitted = await setColorPickerValue(wrapper, '00ff00')
expect(emitted).toBeDefined()
expect(emitted![0]).toContain('#00ff00')
})
it('normalizes rgb() strings to #hex on emit', async () => {
const widget = createMockWidget('#000000')
const wrapper = mountComponent(widget, '#000000')
const emitted = await setColorPickerValue(wrapper, 'rgb(255, 0, 0)')
expect(emitted).toBeDefined()
expect(emitted![0]).toContain('#ff0000')
})
it('normalizes hsb() strings to #hex on emit', async () => {
const widget = createMockWidget('#000000', { format: 'hsb' })
const wrapper = mountComponent(widget, '#000000')
const emitted = await setColorPickerValue(wrapper, 'hsb(120, 100, 100)')
expect(emitted).toBeDefined()
expect(emitted![0]).toContain('#00ff00')
})
it('normalizes HSB object values to #hex on emit', async () => {
const widget = createMockWidget('#000000', { format: 'hsb' })
const wrapper = mountComponent(widget, '#000000')
const emitted = await setColorPickerValue(wrapper, {
h: 240,
s: 100,
b: 100
})
expect(emitted).toBeDefined()
expect(emitted![0]).toContain('#0000ff')
})
})
describe('Component Rendering', () => {
it('renders color picker component', () => {
const widget = createMockWidget('#ff0000')
const wrapper = mountComponent(widget, '#ff0000')
const colorPicker = wrapper.findComponent({ name: 'ColorPicker' })
expect(colorPicker.exists()).toBe(true)
})
it('normalizes display to a single leading #', () => {
// Case 1: model value already includes '#'
let widget = createMockWidget('#ff0000')
let wrapper = mountComponent(widget, '#ff0000')
let colorText = wrapper.find('[data-testid="widget-color-text"]')
expect.soft(colorText.text()).toBe('#ff0000')
// Case 2: model value missing '#'
widget = createMockWidget('ff0000')
wrapper = mountComponent(widget, 'ff0000')
colorText = wrapper.find('[data-testid="widget-color-text"]')
expect.soft(colorText.text()).toBe('#ff0000')
})
it('renders layout field wrapper', () => {
const widget = createMockWidget('#ff0000')
const wrapper = mountComponent(widget, '#ff0000')
const layoutField = wrapper.findComponent({ name: 'WidgetLayoutField' })
expect(layoutField.exists()).toBe(true)
})
it('displays current color value as text', () => {
const widget = createMockWidget('#ff0000')
const wrapper = mountComponent(widget, '#ff0000')
const colorText = wrapper.find('[data-testid="widget-color-text"]')
expect(colorText.text()).toBe('#ff0000')
})
it('updates color text when value changes', async () => {
const widget = createMockWidget('#ff0000')
const wrapper = mountComponent(widget, '#ff0000')
await setColorPickerValue(wrapper, '#00ff00')
// Need to check the local state update
const colorText = wrapper.find('[data-testid="widget-color-text"]')
// Be specific about the displayed value including the leading '#'
expect.soft(colorText.text()).toBe('#00ff00')
})
it('uses default color when no value provided', () => {
const widget = createMockWidget('')
const wrapper = mountComponent(widget, '')
const colorPicker = wrapper.findComponent({ name: 'ColorPicker' })
// Should use the default value from the composable
expect(colorPicker.exists()).toBe(true)
})
})
describe('Readonly Mode', () => {
it('disables color picker when readonly', () => {
const widget = createMockWidget('#ff0000')
const wrapper = mountComponent(widget, '#ff0000', true)
const colorPicker = wrapper.findComponent({ name: 'ColorPicker' })
expect(colorPicker.props('disabled')).toBe(true)
})
it('enables color picker when not readonly', () => {
const widget = createMockWidget('#ff0000')
const wrapper = mountComponent(widget, '#ff0000', false)
const colorPicker = wrapper.findComponent({ name: 'ColorPicker' })
expect(colorPicker.props('disabled')).toBe(false)
})
})
describe('Color Formats', () => {
it('handles valid hex colors', async () => {
const validHexColors = [
'#000000',
'#ffffff',
'#ff0000',
'#00ff00',
'#0000ff',
'#123abc'
]
for (const color of validHexColors) {
const widget = createMockWidget(color)
const wrapper = mountComponent(widget, color)
const colorText = wrapper.find('[data-testid="widget-color-text"]')
expect.soft(colorText.text()).toBe(color)
}
})
it('handles short hex colors', () => {
const widget = createMockWidget('#fff')
const wrapper = mountComponent(widget, '#fff')
const colorText = wrapper.find('[data-testid="widget-color-text"]')
expect(colorText.text()).toBe('#fff')
})
it('passes widget options to color picker', () => {
const colorOptions = {
format: 'hex' as const,
inline: true
}
const widget = createMockWidget('#ff0000', colorOptions)
const wrapper = mountComponent(widget, '#ff0000')
const colorPicker = wrapper.findComponent({ name: 'ColorPicker' })
expect(colorPicker.props('format')).toBe('hex')
expect(colorPicker.props('inline')).toBe(true)
})
})
describe('Widget Layout Integration', () => {
it('passes widget to layout field', () => {
const widget = createMockWidget('#ff0000')
const wrapper = mountComponent(widget, '#ff0000')
const layoutField = wrapper.findComponent({ name: 'WidgetLayoutField' })
expect(layoutField.props('widget')).toEqual(widget)
})
it('maintains proper component structure', () => {
const widget = createMockWidget('#ff0000')
const wrapper = mountComponent(widget, '#ff0000')
// Should have layout field containing label with color picker and text
const layoutField = wrapper.findComponent({ name: 'WidgetLayoutField' })
const label = wrapper.find('label')
const colorPicker = wrapper.findComponent({ name: 'ColorPicker' })
const colorText = wrapper.find('span')
expect(layoutField.exists()).toBe(true)
expect(label.exists()).toBe(true)
expect(colorPicker.exists()).toBe(true)
expect(colorText.exists()).toBe(true)
})
})
describe('Edge Cases', () => {
it('handles empty color value', () => {
const widget = createMockWidget('')
const wrapper = mountComponent(widget, '')
const colorPicker = wrapper.findComponent({ name: 'ColorPicker' })
expect(colorPicker.exists()).toBe(true)
})
it('handles invalid color formats gracefully', async () => {
const widget = createMockWidget('invalid-color')
const wrapper = mountComponent(widget, 'invalid-color')
const colorText = wrapper.find('[data-testid="widget-color-text"]')
expect(colorText.text()).toBe('#000000')
const emitted = await setColorPickerValue(wrapper, 'invalid-color')
expect(emitted).toBeDefined()
expect(emitted![0]).toContain('#000000')
})
it('handles widget with no options', () => {
const widget = createMockWidget('#ff0000')
const wrapper = mountComponent(widget, '#ff0000')
const colorPicker = wrapper.findComponent({ name: 'ColorPicker' })
expect(colorPicker.exists()).toBe(true)
})
})
})

View File

@@ -14,19 +14,26 @@
:pt="{
preview: '!w-full !h-full !border-none'
}"
@update:model-value="onChange"
@update:model-value="onPickerUpdate"
/>
<span class="text-xs">#{{ localValue }}</span>
<span class="text-xs" data-testid="widget-color-text">{{
toHexFromFormat(localValue, format)
}}</span>
</label>
</WidgetLayoutField>
</template>
<script setup lang="ts">
import ColorPicker from 'primevue/colorpicker'
import { computed } from 'vue'
import { computed, ref, watch } from 'vue'
import { useWidgetValue } from '@/composables/graph/useWidgetValue'
import type { SimplifiedWidget } from '@/types/simplifiedWidget'
import {
type ColorFormat,
type HSB,
isColorFormat,
toHexFromFormat
} from '@/utils/colorUtil'
import { cn } from '@/utils/tailwindUtil'
import {
PANEL_EXCLUDED_PROPS,
@@ -36,8 +43,10 @@ import {
import { WidgetInputBaseClass } from './layout'
import WidgetLayoutField from './layout/WidgetLayoutField.vue'
type WidgetOptions = { format?: ColorFormat } & Record<string, unknown>
const props = defineProps<{
widget: SimplifiedWidget<string>
widget: SimplifiedWidget<string, WidgetOptions>
modelValue: string
readonly?: boolean
}>()
@@ -46,14 +55,33 @@ const emit = defineEmits<{
'update:modelValue': [value: string]
}>()
// Use the composable for consistent widget value handling
const { localValue, onChange } = useWidgetValue({
widget: props.widget,
modelValue: props.modelValue,
defaultValue: '#000000',
emit
const format = computed<ColorFormat>(() => {
const optionFormat = props.widget.options?.format
return isColorFormat(optionFormat) ? optionFormat : 'hex'
})
type PickerValue = string | HSB
const localValue = ref<PickerValue>(
toHexFromFormat(
props.modelValue || '#000000',
isColorFormat(props.widget.options?.format)
? props.widget.options.format
: 'hex'
)
)
watch(
() => props.modelValue,
(newVal) => {
localValue.value = toHexFromFormat(newVal || '#000000', format.value)
}
)
function onPickerUpdate(val: unknown) {
localValue.value = val as PickerValue
emit('update:modelValue', toHexFromFormat(val, format.value))
}
// ColorPicker specific excluded props include panel/overlay classes
const COLOR_PICKER_EXCLUDED_PROPS = [...PANEL_EXCLUDED_PROPS] as const

View File

@@ -1,9 +1,20 @@
import { memoize } from 'es-toolkit/compat'
type RGB = { r: number; g: number; b: number }
export interface HSB {
h: number
s: number
b: number
}
type HSL = { h: number; s: number; l: number }
type HSLA = { h: number; s: number; l: number; a: number }
type ColorFormat = 'hex' | 'rgb' | 'rgba' | 'hsl' | 'hsla'
type ColorFormatInternal = 'hex' | 'rgb' | 'rgba' | 'hsl' | 'hsla'
export type ColorFormat = 'hex' | 'rgb' | 'hsb'
interface HSV {
h: number
s: number
v: number
}
export interface ColorAdjustOptions {
lightness?: number
@@ -59,6 +70,65 @@ export function hexToRgb(hex: string): RGB {
return { r, g, b }
}
export function rgbToHex({ r, g, b }: RGB): string {
const toHex = (n: number) =>
Math.max(0, Math.min(255, Math.round(n)))
.toString(16)
.padStart(2, '0')
return `#${toHex(r)}${toHex(g)}${toHex(b)}`
}
export function hsbToRgb({ h, s, b }: HSB): RGB {
// Normalize
const hh = ((h % 360) + 360) % 360
const ss = Math.max(0, Math.min(100, s)) / 100
const vv = Math.max(0, Math.min(100, b)) / 100
const c = vv * ss
const x = c * (1 - Math.abs(((hh / 60) % 2) - 1))
const m = vv - c
let rp = 0,
gp = 0,
bp = 0
if (hh < 60) {
rp = c
gp = x
bp = 0
} else if (hh < 120) {
rp = x
gp = c
bp = 0
} else if (hh < 180) {
rp = 0
gp = c
bp = x
} else if (hh < 240) {
rp = 0
gp = x
bp = c
} else if (hh < 300) {
rp = x
gp = 0
bp = c
} else {
rp = c
gp = 0
bp = x
}
return {
r: Math.floor((rp + m) * 255),
g: Math.floor((gp + m) * 255),
b: Math.floor((bp + m) * 255)
}
}
/**
* Normalize various color inputs (hex, rgb/rgba, hsl/hsla, hsb string/object)
* into lowercase #rrggbb. Falls back to #000000 on invalid inputs.
*/
export function parseToRgb(color: string): RGB {
const format = identifyColorFormat(color)
if (!format) return { r: 0, g: 0, b: 0 }
@@ -112,7 +182,7 @@ export function parseToRgb(color: string): RGB {
}
}
const identifyColorFormat = (color: string): ColorFormat | null => {
const identifyColorFormat = (color: string): ColorFormatInternal | null => {
if (!color) return null
if (color.startsWith('#') && (color.length === 4 || color.length === 7))
return 'hex'
@@ -133,7 +203,73 @@ const isHSLA = (color: unknown): color is HSLA => {
)
}
function parseToHSLA(color: string, format: ColorFormat): HSLA | null {
export function isColorFormat(v: unknown): v is ColorFormat {
return v === 'hex' || v === 'rgb' || v === 'hsb'
}
function isHSBObject(v: unknown): v is HSB {
if (!v || typeof v !== 'object') return false
const rec = v as Record<string, unknown>
return (
typeof rec.h === 'number' &&
Number.isFinite(rec.h) &&
typeof rec.s === 'number' &&
Number.isFinite(rec.s) &&
typeof (rec as Record<string, unknown>).b === 'number' &&
Number.isFinite((rec as Record<string, number>).b!)
)
}
function isHSVObject(v: unknown): v is HSV {
if (!v || typeof v !== 'object') return false
const rec = v as Record<string, unknown>
return (
typeof rec.h === 'number' &&
Number.isFinite(rec.h) &&
typeof rec.s === 'number' &&
Number.isFinite(rec.s) &&
typeof (rec as Record<string, unknown>).v === 'number' &&
Number.isFinite((rec as Record<string, number>).v!)
)
}
export function toHexFromFormat(val: unknown, format: ColorFormat): string {
if (format === 'hex' && typeof val === 'string') {
const raw = val.trim().toLowerCase()
if (!raw) return '#000000'
if (/^[0-9a-f]{3}$/.test(raw)) return `#${raw}`
if (/^#[0-9a-f]{3}$/.test(raw)) return raw
if (/^[0-9a-f]{6}$/.test(raw)) return `#${raw}`
if (/^#[0-9a-f]{6}$/.test(raw)) return raw
return '#000000'
}
if (format === 'rgb' && typeof val === 'string') {
const rgb = parseToRgb(val)
return rgbToHex(rgb).toLowerCase()
}
if (format === 'hsb') {
if (isHSBObject(val)) {
return rgbToHex(hsbToRgb(val)).toLowerCase()
}
if (isHSVObject(val)) {
const { h, s, v } = val
return rgbToHex(hsbToRgb({ h, s, b: v })).toLowerCase()
}
if (typeof val === 'string') {
const nums = val.match(/\d+(?:\.\d+)?/g)?.map(Number) || []
if (nums.length >= 3) {
return rgbToHex(
hsbToRgb({ h: nums[0], s: nums[1], b: nums[2] })
).toLowerCase()
}
}
}
return '#000000'
}
function parseToHSLA(color: string, format: ColorFormatInternal): HSLA | null {
let match: RegExpMatchArray | null
switch (format) {

View File

@@ -1,6 +1,12 @@
import { describe, expect, it, vi } from 'vitest'
import { adjustColor } from '@/utils/colorUtil'
import {
adjustColor,
hexToRgb,
hsbToRgb,
parseToRgb,
rgbToHex
} from '@/utils/colorUtil'
interface ColorTestCase {
hex: string
@@ -55,6 +61,74 @@ const colors: Record<string, ColorTestCase> = {
const formats: ColorFormat[] = ['hex', 'rgb', 'rgba', 'hsl', 'hsla']
describe('colorUtil conversions', () => {
describe('hexToRgb / rgbToHex', () => {
it('converts 6-digit hex to RGB', () => {
expect(hexToRgb('#ff0000')).toEqual({ r: 255, g: 0, b: 0 })
expect(hexToRgb('#00ff00')).toEqual({ r: 0, g: 255, b: 0 })
expect(hexToRgb('#0000ff')).toEqual({ r: 0, g: 0, b: 255 })
})
it('converts 3-digit hex to RGB', () => {
expect(hexToRgb('#f00')).toEqual({ r: 255, g: 0, b: 0 })
expect(hexToRgb('#0f0')).toEqual({ r: 0, g: 255, b: 0 })
expect(hexToRgb('#00f')).toEqual({ r: 0, g: 0, b: 255 })
})
it('converts RGB to lowercase #hex and clamps values', () => {
expect(rgbToHex({ r: 255, g: 0, b: 0 })).toBe('#ff0000')
expect(rgbToHex({ r: 0, g: 255, b: 0 })).toBe('#00ff00')
expect(rgbToHex({ r: 0, g: 0, b: 255 })).toBe('#0000ff')
// out-of-range should clamp
expect(rgbToHex({ r: -10, g: 300, b: 16 })).toBe('#00ff10')
})
it('round-trips #hex -> rgb -> #hex', () => {
const hex = '#123abc'
expect(rgbToHex(hexToRgb(hex))).toBe('#123abc')
})
})
describe('parseToRgb', () => {
it('parses #hex', () => {
expect(parseToRgb('#ff0000')).toEqual({ r: 255, g: 0, b: 0 })
})
it('parses rgb()/rgba()', () => {
expect(parseToRgb('rgb(255, 0, 0)')).toEqual({ r: 255, g: 0, b: 0 })
expect(parseToRgb('rgba(255,0,0,0.5)')).toEqual({ r: 255, g: 0, b: 0 })
})
it('parses hsl()/hsla()', () => {
expect(parseToRgb('hsl(0, 100%, 50%)')).toEqual({ r: 255, g: 0, b: 0 })
const green = parseToRgb('hsla(120, 100%, 50%, 0.7)')
expect(green.r).toBe(0)
expect(green.g).toBe(255)
expect(green.b).toBe(0)
})
})
describe('hsbToRgb', () => {
it('converts HSB to primary RGB colors', () => {
expect(hsbToRgb({ h: 0, s: 100, b: 100 })).toEqual({ r: 255, g: 0, b: 0 })
expect(hsbToRgb({ h: 120, s: 100, b: 100 })).toEqual({
r: 0,
g: 255,
b: 0
})
expect(hsbToRgb({ h: 240, s: 100, b: 100 })).toEqual({
r: 0,
g: 0,
b: 255
})
})
it('handles non-100 brightness and clamps/normalizes input', () => {
const rgb = hsbToRgb({ h: 360, s: 150, b: 50 })
expect(rgbToHex(rgb)).toBe('#7f0000')
})
})
})
describe('colorUtil - adjustColor', () => {
const runAdjustColorTests = (
color: ColorTestCase,