Conversation
Closed
2 tasks
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds color UI primitives (ColorSwatch, ColorSwatchPicker, ColorArea, ColorSlider, ColorField), a comprehensive color utility library (types, parse/convert/gradient/channel utils), tests, docs, demos, stories, registry and export updates, and small behavior/context tweaks in NumberField and date input handling. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as User
participant App as App (v-model)
participant Root as ColorSwatchPickerRoot
participant Item as ColorSwatchPickerItem
participant Swatch as ColorSwatch
Root->>Item: render item(value) and provide color context
Item->>Swatch: inject color from context / set CSS var
User->>Item: click or keyboard select
Item->>Root: set selection state (data-state)
Root->>App: emit update:modelValue (selection)
App->>Root: v-model updates (controlled mode)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
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.gh.lumenfield.work/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/core/src/Slider/SliderHorizontal.vue (1)
82-83:⚠️ Potential issue | 🔴 CriticalCritical bug: Ref assignment without
.value.The code directly assigns
undefinedto the ref variables instead of their values. This breaks reactivity and will cause runtime errors when line 28 tries to accessrectRef.valueafter a slide-end event.🐛 Proposed fix
`@slide-end`="() => { - rectRef = undefined; - offsetPosition = undefined + rectRef.value = undefined; + offsetPosition.value = undefined emits('slideEnd') }"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/Slider/SliderHorizontal.vue` around lines 82 - 83, The refs rectRef and offsetPosition are being overwritten instead of updating their inner values, breaking reactivity and causing runtime errors when code later accesses rectRef.value (e.g., after slide-end). Fix by assigning undefined to rectRef.value and offsetPosition.value rather than reassigning the ref variables themselves (update the assignments in SliderHorizontal.vue where rectRef and offsetPosition are set to undefined), ensuring any reactive consumers still reference the same ref objects.packages/core/src/shared/date/useDateField.ts (1)
872-902:⚠️ Potential issue | 🟠 MajorReinstate
preventDefaultfor handled segment keys.At lines 872–902, handled keys are no longer prevented from triggering default browser behavior. Since segments are
contenteditable(line 38), native text editing can run alongside your controlled state updates and cause UI/state drift. This was previously handled in earlier commits but has been removed, creating a UX regression.💡 Proposed fix
function handleSegmentKeydown(e: KeyboardEvent) { const disabled = props.disabled.value const readonly = props.readonly.value if (disabled || readonly) return + + // Prevent native contenteditable mutations for keys handled by segment logic. + // Allow TAB and modifier combos for standard browser navigation. + if (isAcceptableSegmentKey(e.key) && e.key !== kbd.TAB && !e.ctrlKey && !e.metaKey && !e.altKey) + e.preventDefault() + const segmentKeydownHandlers = { day: handleDaySegmentKeydown, month: handleMonthSegmentKeydown, year: handleYearSegmentKeydown,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/shared/date/useDateField.ts` around lines 872 - 902, The handler handleSegmentKeydown no longer calls preventDefault for keys it handles, allowing native contenteditable editing to conflict with our controlled updates; update handleSegmentKeydown so that any key processed by segmentKeydownHandlers (look up by props.part) or any key for which isAcceptableSegmentKey(e.key) returns true has e.preventDefault() called before performing state mutations (e.g., before calling props.placeholder.value.set(...) and assigning props.modelValue.value), and ensure this also applies to arrow/Tab/Shift handling paths where you intend to intercept the key.
🧹 Nitpick comments (8)
packages/core/src/Select/SelectScrollButtonImpl.vue (1)
1-3: Remove the empty non-setup<script>block.This block is inert and adds SFC noise with no behavior change. Prefer keeping only
<script setup lang="ts">unless you need classic script options/exports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/Select/SelectScrollButtonImpl.vue` around lines 1 - 3, Remove the empty non-setup <script> block from the SelectScrollButtonImpl.vue SFC; keep only the existing <script setup lang="ts"> form (or add it if missing) so there is no inert classic <script> section left, ensuring the component uses the single-file setup script style without redundant empty blocks.packages/core/src/Menu/MenuRootContentNonModal.vue (1)
1-3: Remove inert empty<script>block.Lines 1-3 add no declarations or side effects, so this block is just noise. Prefer keeping only
<script setup lang="ts">for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/Menu/MenuRootContentNonModal.vue` around lines 1 - 3, Remove the inert empty <script lang="ts"> block at the top of MenuRootContentNonModal.vue; it adds no declarations or side effects and is redundant when you already use <script setup lang="ts">—simply delete the empty <script> tag so only the <script setup lang="ts"> block remains.packages/core/src/Slider/SliderHorizontal.vue (1)
1-2: Clarify or remove the empty script block.The empty
<script lang="ts">block serves no functional purpose. While the AI summary mentions this is for "formatting/structural consistency" across multiple files, it's unclear why this is necessary.If this is required for build tooling, linting, or a specific framework reason, please add a comment explaining its purpose. Otherwise, consider removing it to reduce code clutter.
🧹 Proposed cleanup
If not required:
-<script lang="ts"> -</script> - <script setup lang="ts">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/Slider/SliderHorizontal.vue` around lines 1 - 2, The file contains an empty <script lang="ts"> block in SliderHorizontal.vue; either remove that empty script block to eliminate dead markup, or keep it but add a one-line comment inside explaining why it's required for build tooling/linters/framework consistency (e.g., "keep empty script for tooling/formatting consistency") so future readers know it's intentional; update the file around the <script lang="ts"> block accordingly.packages/core/src/shared/macro.ts (1)
1-11: Extract shared modifier check; current code works across layouts but refactor improves clarity.The code is already layout-agnostic for letter keys—
event.keyis consistent for 'c', 'x', 'v' across keyboard layouts. That said, extracting the repeated(event.ctrlKey || event.metaKey)check reduces duplication and improves maintainability:🔧 Suggested refactor
+function hasPrimaryModifier(event: KeyboardEvent) { + return event.ctrlKey || event.metaKey +} + export function isCut(event: KeyboardEvent) { - return (event.key === 'x' || event.key === 'X') && (event.ctrlKey || event.metaKey) + return (event.key === 'x' || event.key === 'X') && hasPrimaryModifier(event) } export function isCopy(event: KeyboardEvent) { - return (event.key === 'c' || event.key === 'C') && (event.ctrlKey || event.metaKey) + return (event.key === 'c' || event.key === 'C') && hasPrimaryModifier(event) } export function isPaste(event: KeyboardEvent) { - return (event.key === 'v' || event.key === 'V') && (event.ctrlKey || event.metaKey) + return (event.key === 'v' || event.key === 'V') && hasPrimaryModifier(event) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/shared/macro.ts` around lines 1 - 11, The three helper functions isCut, isCopy, and isPaste duplicate the modifier check (event.ctrlKey || event.metaKey); extract that into a shared predicate (e.g., hasCtrlOrMeta or isModifierPressed) and use it inside isCut/isCopy/isPaste to reduce duplication and improve clarity while keeping the same behavior and signatures.packages/core/src/ColorSwatchPicker/ColorSwatchPicker.test.ts (1)
13-13: Prefer DOM cleanup API overinnerHTMLassignment in tests.Line 13 can use
replaceChildren()for clearer intent and to avoid recurring XSS-lint noise.Proposed refactor
- document.body.innerHTML = '' + document.body.replaceChildren()🤖 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` at line 13, The test uses a raw assignment to document.body.innerHTML to clear the DOM; replace that with the DOM cleanup API by calling document.body.replaceChildren() in the ColorSwatchPicker.test.ts teardown/setup so the body is cleared without assigning innerHTML and to avoid XSS-lint warnings (locate the document.body.innerHTML = '' occurrence and swap it to use replaceChildren()).packages/core/src/ColorSwatch/ColorSwatch.test.ts (1)
6-16: Consider expanding test coverage.The current tests verify basic rendering and accessibility, which is a good starting point. Consider adding tests for:
- Custom
labelprop overriding derived color name- Invalid color values triggering fallback behavior
colorContrastdata attribute values🤖 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 - 16, Add unit tests to ColorSwatch.test.ts that cover (1) setting the label prop on the ColorSwatch component to ensure it overrides the derived color name by mounting ColorSwatch with { props: { label: 'Custom' } } and asserting the rendered label text equals 'Custom', (2) supplying invalid color values by mounting ColorSwatch with an obviously invalid color (e.g., 'invalid-color') and asserting the component falls back to the expected fallback behavior (e.g., presence of a fallback CSS class, default color token, or no color value in the swatch element), and (3) verifying the color contrast metadata by mounting ColorSwatch with a known color and asserting the DOM contains the expected data attribute for contrast (check element.getAttribute('data-color-contrast') or the data-color-contrast attribute value) to ensure colorContrast is computed and rendered correctly.packages/core/src/shared/color/utils.ts (2)
74-74: Add explicit return type annotation.The function is missing an explicit return type annotation for consistency with the other exported functions in this module.
Suggested fix
-export function getColorName(hex: string) { +export function getColorName(hex: string): string {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/shared/color/utils.ts` at line 74, The exported function getColorName(hex: string) lacks an explicit return type; update its signature to include the correct return type (e.g., getColorName(hex: string): string | undefined) to match its actual return values and maintain consistency with other exports in the module.
111-112: Hue range for "blue" may produce unexpected results.Pure blue (
#0000FF) has a hue of 240°, which falls into the 225-255 range mapped to "blue-violet" rather than "blue". This causes the test at line 96 inutils.test.tsto expect'vibrant blue-violet'for#0000FF, which may surprise users expecting pure blue to return "blue".Consider adjusting the hue boundaries so that 240° falls within "blue":
Suggested adjustment
else if (h < 195) baseName = 'cyan' - else if (h < 225) - baseName = 'blue' - else if (h < 255) + else if (h < 250) + baseName = 'blue' + else if (h < 280) baseName = 'blue-violet'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/shared/color/utils.ts` around lines 111 - 112, The hue boundary that assigns baseName='blue' is too low (currently using "else if (h < 225)"), which puts pure blue (h=240) into the "blue-violet" bucket; update the hue comparison in the color-mapping logic (the branch that sets baseName to 'blue', using the h variable and baseName) to raise the upper bound so that h=240 falls into 'blue' (e.g., change the threshold from 225 to a value >240 such as 245 or 241) so pure blue maps to 'blue' while preserving the subsequent "blue-violet" range.
🤖 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/ColorSwatchPickerItem.md`:
- Around line 33-35: The event description for the 'select' entry is
grammatically awkward; update the 'description' value for the 'select' metadata
so it reads clearly (e.g., "Event handler called when an item is selected. It
can be prevented by calling event.preventDefault().") and ensure it still
references the same event type ('type': '[event: SelectEvent<AcceptableValue>]')
and uses proper punctuation and the ()=> style for preventDefault in the text.
In `@docs/content/meta/ColorSwatchPickerRoot.md`:
- Around line 45-47: The API docs string for the prop 'modelValue' contains the
nonstandard word "binded"; update the source metadata or JSDoc/comment that
feeds generation (where 'modelValue' description is defined) to replace "binded
with <code>v-model</code>" with "bound with <code>v-model</code>" so regenerated
docs show the corrected wording for the 'modelValue' description.
In `@packages/core/.gitignore`:
- Line 15: The packages/core/.gitignore currently contains an explicit "dist"
entry which causes packages/core/dist/ to be ignored while root-level dist/
remains trackable; if the intent is to track distribution/build artifacts from
packages/core, remove the "dist" line from packages/core/.gitignore (or replace
it with a more specific ignore like /dist/ at root or add an explicit negate
rule) so packages/core/dist/ can be committed; otherwise, if the divergence is
intentional, document the strategy in the repo README or add a comment in
packages/core/.gitignore explaining why packages/core/dist/ must be ignored.
In `@packages/core/package.json`:
- Line 2: The package.json "name" field was unintentionally changed to
"@explorate/reka-ui"; revert the "name" value in packages/core/package.json back
to its original package identity (restore the previous string in the "name" key)
to avoid breaking consumers and publishing, or if the rename is intentional
update all related locations (workspace root/package.json, lockfiles, CI/publish
scripts, docs, and any imports) consistently; look for the "name" key in
packages/core/package.json and ensure it matches the rest of the repo's package
identity before merging.
In `@packages/core/src/ColorSwatchPicker/ColorSwatchPickerRoot.vue`:
- Around line 17-31: The defaultValue prop is always set to an empty string
which breaks multi-select mode; update the props/defaults logic so that when the
component is in multiple mode (props.multiple === true) the defaultValue is an
empty array instead of ''. Change the default for defaultValue in the props
initializer (where props and withDefaults are defined) and ensure
useVModel(modelValue, 'modelValue', emits, { defaultValue: props.defaultValue,
... }) receives an initial value whose type matches the multiple flag so
modelValue/useVModel and ColorSwatchPickerRootProps remain consistent.
In `@packages/core/src/Menu/MenuRootContentModal.vue`:
- Around line 1-3: The empty <script lang="ts"></script> block was added to
MenuRootContentModal.vue without explanation; either remove this empty block if
it's not required for production components, or add a brief comment in the file
explaining why the pattern is necessary (e.g., build/tooling/TS type scope when
using <script setup>) so future reviewers understand the intent; specifically
update MenuRootContentModal.vue to either delete the empty <script lang="ts">
block or leave it and add a one-line comment above it referencing the technical
reason and link to any shared convention/docs used elsewhere in the codebase.
In `@packages/core/src/Progress/ProgressRoot.vue`:
- Line 154: The aria-valuetext binding uses getValueLabel but should honor the
public getValueText prop; update the aria-valuetext expression in
ProgressRoot.vue to use getValueText(modelValue, max) when provided and fall
back to getValueLabel(modelValue, max) (and undefined if modelValue is not a
number), so consumers supplying getValueText affect the accessible text; ensure
references to modelValue, max, getValueText, and getValueLabel are used
accordingly.
In `@packages/core/src/Tooltip/TooltipContentHoverable.vue`:
- Around line 1-2: Remove the redundant empty <script lang="ts"></script> block
that precedes the actual <script setup lang="ts"> in TooltipContentHoverable.vue
(and apply the same change consistently across the other ~246 files); if there
is a build/tooling reason to keep it, instead add a short comment inside the
empty block explaining that reason (e.g., "Required for X tooling/type
generation") so future readers know why it's present—locate the SFCs by the
presence of both <script lang="ts"> and <script setup lang="ts"> and either
delete the empty non-setup script or replace it with a one-line explanatory
comment.
---
Outside diff comments:
In `@packages/core/src/shared/date/useDateField.ts`:
- Around line 872-902: The handler handleSegmentKeydown no longer calls
preventDefault for keys it handles, allowing native contenteditable editing to
conflict with our controlled updates; update handleSegmentKeydown so that any
key processed by segmentKeydownHandlers (look up by props.part) or any key for
which isAcceptableSegmentKey(e.key) returns true has e.preventDefault() called
before performing state mutations (e.g., before calling
props.placeholder.value.set(...) and assigning props.modelValue.value), and
ensure this also applies to arrow/Tab/Shift handling paths where you intend to
intercept the key.
In `@packages/core/src/Slider/SliderHorizontal.vue`:
- Around line 82-83: The refs rectRef and offsetPosition are being overwritten
instead of updating their inner values, breaking reactivity and causing runtime
errors when code later accesses rectRef.value (e.g., after slide-end). Fix by
assigning undefined to rectRef.value and offsetPosition.value rather than
reassigning the ref variables themselves (update the assignments in
SliderHorizontal.vue where rectRef and offsetPosition are set to undefined),
ensuring any reactive consumers still reference the same ref objects.
---
Nitpick comments:
In `@packages/core/src/ColorSwatch/ColorSwatch.test.ts`:
- Around line 6-16: Add unit tests to ColorSwatch.test.ts that cover (1) setting
the label prop on the ColorSwatch component to ensure it overrides the derived
color name by mounting ColorSwatch with { props: { label: 'Custom' } } and
asserting the rendered label text equals 'Custom', (2) supplying invalid color
values by mounting ColorSwatch with an obviously invalid color (e.g.,
'invalid-color') and asserting the component falls back to the expected fallback
behavior (e.g., presence of a fallback CSS class, default color token, or no
color value in the swatch element), and (3) verifying the color contrast
metadata by mounting ColorSwatch with a known color and asserting the DOM
contains the expected data attribute for contrast (check
element.getAttribute('data-color-contrast') or the data-color-contrast attribute
value) to ensure colorContrast is computed and rendered correctly.
In `@packages/core/src/ColorSwatchPicker/ColorSwatchPicker.test.ts`:
- Line 13: The test uses a raw assignment to document.body.innerHTML to clear
the DOM; replace that with the DOM cleanup API by calling
document.body.replaceChildren() in the ColorSwatchPicker.test.ts teardown/setup
so the body is cleared without assigning innerHTML and to avoid XSS-lint
warnings (locate the document.body.innerHTML = '' occurrence and swap it to use
replaceChildren()).
In `@packages/core/src/Menu/MenuRootContentNonModal.vue`:
- Around line 1-3: Remove the inert empty <script lang="ts"> block at the top of
MenuRootContentNonModal.vue; it adds no declarations or side effects and is
redundant when you already use <script setup lang="ts">—simply delete the empty
<script> tag so only the <script setup lang="ts"> block remains.
In `@packages/core/src/Select/SelectScrollButtonImpl.vue`:
- Around line 1-3: Remove the empty non-setup <script> block from the
SelectScrollButtonImpl.vue SFC; keep only the existing <script setup lang="ts">
form (or add it if missing) so there is no inert classic <script> section left,
ensuring the component uses the single-file setup script style without redundant
empty blocks.
In `@packages/core/src/shared/color/utils.ts`:
- Line 74: The exported function getColorName(hex: string) lacks an explicit
return type; update its signature to include the correct return type (e.g.,
getColorName(hex: string): string | undefined) to match its actual return values
and maintain consistency with other exports in the module.
- Around line 111-112: The hue boundary that assigns baseName='blue' is too low
(currently using "else if (h < 225)"), which puts pure blue (h=240) into the
"blue-violet" bucket; update the hue comparison in the color-mapping logic (the
branch that sets baseName to 'blue', using the h variable and baseName) to raise
the upper bound so that h=240 falls into 'blue' (e.g., change the threshold from
225 to a value >240 such as 245 or 241) so pure blue maps to 'blue' while
preserving the subsequent "blue-violet" range.
In `@packages/core/src/shared/macro.ts`:
- Around line 1-11: The three helper functions isCut, isCopy, and isPaste
duplicate the modifier check (event.ctrlKey || event.metaKey); extract that into
a shared predicate (e.g., hasCtrlOrMeta or isModifierPressed) and use it inside
isCut/isCopy/isPaste to reduce duplication and improve clarity while keeping the
same behavior and signatures.
In `@packages/core/src/Slider/SliderHorizontal.vue`:
- Around line 1-2: The file contains an empty <script lang="ts"> block in
SliderHorizontal.vue; either remove that empty script block to eliminate dead
markup, or keep it but add a one-line comment inside explaining why it's
required for build tooling/linters/framework consistency (e.g., "keep empty
script for tooling/formatting consistency") so future readers know it's
intentional; update the file around the <script lang="ts"> block accordingly.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (63)
.gitignoredocs/.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/docs/guides/migration.mddocs/content/meta/ColorSwatch.mddocs/content/meta/ColorSwatchPickerItem.mddocs/content/meta/ColorSwatchPickerItemIndicator.mddocs/content/meta/ColorSwatchPickerItemSwatch.mddocs/content/meta/ColorSwatchPickerRoot.mdpackages/core/.gitignorepackages/core/constant/components.tspackages/core/package.jsonpackages/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/ConfigProvider/_ConfigProvider.vuepackages/core/src/ContextMenu/ContextMenuPortal.vuepackages/core/src/Dialog/DialogContentModal.vuepackages/core/src/Dialog/DialogContentNonModal.vuepackages/core/src/FocusGuards/FocusGuards.vuepackages/core/src/Listbox/ListboxRoot.vuepackages/core/src/Menu/MenuRootContentModal.vuepackages/core/src/Menu/MenuRootContentNonModal.vuepackages/core/src/NumberField/NumberFieldRoot.vuepackages/core/src/Popover/PopoverContentModal.vuepackages/core/src/Popover/PopoverContentNonModal.vuepackages/core/src/Progress/ProgressRoot.vuepackages/core/src/ScrollArea/ScrollAreaCornerImpl.vuepackages/core/src/ScrollArea/ScrollAreaScrollbarX.vuepackages/core/src/ScrollArea/ScrollAreaScrollbarY.vuepackages/core/src/Select/BubbleSelect.vuepackages/core/src/Select/SelectProvider.vuepackages/core/src/Select/SelectScrollButtonImpl.vuepackages/core/src/Slider/SliderHorizontal.vuepackages/core/src/Slider/SliderVertical.vuepackages/core/src/Switch/_Switch.vuepackages/core/src/Toast/FocusProxy.vuepackages/core/src/Toast/ToastAnnounce.vuepackages/core/src/Tooltip/TooltipContentHoverable.vuepackages/core/src/VisuallyHidden/VisuallyHiddenInput.vuepackages/core/src/index.tspackages/core/src/shared/color/index.tspackages/core/src/shared/color/utils.test.tspackages/core/src/shared/color/utils.tspackages/core/src/shared/date/useDateField.tspackages/core/src/shared/macro.ts
💤 Files with no reviewable changes (1)
- .gitignore
commit: |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (6)
packages/core/src/ColorField/story/_ColorField.vue-20-20 (1)
20-20:⚠️ Potential issue | 🟡 Minor
defaultValuechanges won’t reflect after mount in the story.Line 20 initializes local state once; interactive prop updates (e.g., story controls) won’t update
color.Suggested fix
- import { ref } from 'vue' + import { ref, watch } from 'vue' ... - const color = ref(props.defaultValue ?? '#ff0000') + const color = ref('#ff0000') + + watch( + () => props.defaultValue, + (value) => { + color.value = value ?? '#ff0000' + }, + { immediate: true }, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/ColorField/story/_ColorField.vue` at line 20, The local ref color is initialized from props.defaultValue once (const color = ref(props.defaultValue ?? '#ff0000')), so updates to the prop (e.g., Storybook controls) won't propagate; change this by syncing the ref with the prop—either replace the standalone ref with a computed getter/setter that reads/writes props.defaultValue or add a watch on props.defaultValue that updates color when the prop changes; reference the existing symbols color, props.defaultValue, and the use of ref to locate and update the implementation.packages/core/src/shared/color/parse.ts-74-75 (1)
74-75:⚠️ Potential issue | 🟡 MinorAnchor parser regexes to reject trailing garbage.
Current patterns accept strings like
rgb(1,2,3)fooas valid. Anchoring with^...$makes validation strict and predictable.Suggested fix
- const match = rgb.match(/rgba?\(\s*([\d.]+)\s*,\s*([\d.]+)\s*,\s*([\d.]+)\s*(?:,\s*([\d.]+)\s*)?\)/) + const match = rgb.match(/^rgba?\(\s*([\d.]+)\s*,\s*([\d.]+)\s*,\s*([\d.]+)\s*(?:,\s*([\d.]+)\s*)?\)$/) - const match = hsl.match(/hsla?\(\s*([\d.]+)\s*,\s*([\d.]+)%\s*,\s*([\d.]+)%\s*(?:,\s*([\d.]+)\s*)?\)/) + const match = hsl.match(/^hsla?\(\s*([\d.]+)\s*,\s*([\d.]+)%\s*,\s*([\d.]+)%\s*(?:,\s*([\d.]+)\s*)?\)$/) - const match = hsb.match(/^hs(?:b|v)a?\(\s*([\d.]+)\s*,\s*([\d.]+)%?\s*,\s*([\d.]+)%?\s*(?:,\s*([\d.]+)\s*)?\)$/) + const match = hsb.match(/^hs(?:b|v)a?\(\s*([\d.]+)\s*,\s*([\d.]+)%?\s*,\s*([\d.]+)%?\s*(?:,\s*([\d.]+)\s*)?\)$/)Also applies to: 89-90, 104-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/shared/color/parse.ts` around lines 74 - 75, The regexes that parse rgb/rgba (the one assigning match from rgb.match(/rgba?\(.../)) and the two other similar regexes later in this file are too permissive and accept trailing characters (e.g., "rgb(1,2,3)foo"); update each of those regex literals to be anchored with ^ at the start and $ at the end so they only match the entire input string (modify the regex passed to String.match in the rgb/rgba parsing expression and the two analogous match expressions later in the file).packages/core/src/ColorSwatch/ColorSwatch.vue-64-74 (1)
64-74:⚠️ Potential issue | 🟡 MinorAvoid contrast resolution for no-color states to prevent false warnings.
When swatch is empty/transparent, contrast lookup is unnecessary and can produce noisy DEV warnings.
Suggested fix
const colorContrast = computed(() => { + if (isNoColor.value || !colorString.value) + return undefined + try { return getColorContrast(colorString.value) }🤖 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 64 - 74, The computed colorContrast should skip calling getColorContrast when the swatch represents "no color" to avoid DEV warnings; update the colorContrast computed (which reads colorString.value and calls getColorContrast) to first check for empty/falsy values and explicit transparent states (e.g., empty string or 'transparent', and any other project-specific no-color sentinel) and immediately return undefined in those cases, otherwise call getColorContrast inside the try/catch as before.packages/core/src/ColorSlider/ColorSlider.test.ts-197-212 (1)
197-212:⚠️ Potential issue | 🟡 Minor
custom steptest does not validate step behavior yet.The test currently re-checks range bounds, but never verifies that keyboard interaction moves by
step.🧪 Strengthen the test
-describe('custom step prop', () => { - it('should use custom step value', () => { +describe('custom step prop', () => { + it('should increment by the custom step on ArrowRight', async () => { const wrapper = mount(ColorSlider, { props: { defaultValue: '#ff0000', channel: 'hue', colorSpace: 'hsl', step: 10, }, }) const thumb = wrapper.find('[role="slider"]') - expect(thumb.exists()).toBe(true) - // The SliderRoot should receive step=10 - expect(thumb.attributes('aria-valuemin')).toBe('0') - expect(thumb.attributes('aria-valuemax')).toBe('360') + const before = Number(thumb.attributes('aria-valuenow')) + await thumb.trigger('keydown', { key: 'ArrowRight' }) + const after = Number(thumb.attributes('aria-valuenow')) + expect(after - before).toBe(10) }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/ColorSlider/ColorSlider.test.ts` around lines 197 - 212, The test only checks min/max but not that keyboard movement respects the step prop; update the 'custom step prop' test in ColorSlider.test.ts to simulate keyboard interaction on the slider thumb (the element found by wrapper.find('[role="slider"]')) after mounting ColorSlider with step: 10 and initial value '#ff0000', focus the thumb, trigger a relevant keydown (e.g., 'ArrowRight' or 'ArrowUp' depending on the hue channel) and assert that thumb.attributes('aria-valuenow') changed from the initial value to initial+10 (e.g., '0' -> '10'), ensuring the SliderRoot/ColorSlider keyboard handling uses the provided step prop.packages/core/src/ColorSlider/ColorSlider.test.ts-13-13 (1)
13-13:⚠️ Potential issue | 🟡 Minor
hasPointerCapturemock return type is incorrect.
mockImplementation(id => id)returns the pointerId (a number), butHTMLElement.hasPointerCapture()must return a boolean according to the DOM specification. This can hide contract violations in tests.✅ Minimal fix
-window.HTMLElement.prototype.hasPointerCapture = vi.fn().mockImplementation(id => id) +window.HTMLElement.prototype.hasPointerCapture = vi.fn().mockReturnValue(false)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/ColorSlider/ColorSlider.test.ts` at line 13, The mock for window.HTMLElement.prototype.hasPointerCapture returns a number (id) which violates the DOM contract expecting a boolean; update the mock implementation in ColorSlider.test (the hasPointerCapture mock) to return a boolean instead (e.g., return !!id or a fixed true/false) so tests reflect the correct return type for HTMLElement.hasPointerCapture.packages/core/src/shared/color/gradient.ts-147-151 (1)
147-151:⚠️ Potential issue | 🟡 MinorBrightness area gradient is inverted on Y-axis fallback.
Foraxis: 'y', current order places black at the top instead of bottom.🔧 Suggested fix
case 'brightness': { - // For HSB: Transparent to black (overlay on pure hue) - // Top (100% brightness) = transparent (shows pure hue), Bottom (0% brightness) = black - return `linear-gradient(${direction}, rgba(0, 0, 0, 0), rgba(0, 0, 0, 1))` + // For HSB Y axis: bottom should be black, top should be transparent. + return axis === 'y' + ? `linear-gradient(${direction}, rgba(0, 0, 0, 1), rgba(0, 0, 0, 0))` + : `linear-gradient(${direction}, rgba(0, 0, 0, 0), rgba(0, 0, 0, 1))` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/shared/color/gradient.ts` around lines 147 - 151, In the 'brightness' branch inside gradient.ts (case 'brightness') the fallback gradient is currently returned as linear-gradient(${direction}, rgba(0,0,0,0), rgba(0,0,0,1)) which places black at the top for axis === 'y'; update the logic so the RGBA stop order is swapped when axis is 'y' (or compute a corrected direction) so that for vertical gradients the transparent stop comes first at the top and the black stop is at the bottom; modify the code around the 'direction' usage in the case 'brightness' block to conditionally return either linear-gradient(${direction}, rgba(0,0,0,0), rgba(0,0,0,1)) or linear-gradient(${direction}, rgba(0,0,0,1), rgba(0,0,0,0)) depending on axis === 'y'.
🧹 Nitpick comments (9)
packages/core/src/ColorPicker/story/ColorPickerDemo.story.vue (1)
11-29: Consider data-driving variant rendering to reduce duplication.The repeated
Variantblocks can be collapsed into a small config array for easier maintenance when adding/removing cases.♻️ Proposed refactor
<script setup lang="ts"> import ColorPicker from './_ColorPicker.vue' + +const variants = [ + { title: 'Complete Color Picker', value: '#7f007f' }, + { title: 'Red Color', value: '#ff0000' }, + { title: 'Green Color', value: '#00ff00' }, + { title: 'Blue Color', value: '#0000ff' }, + { title: 'Semi-transparent', value: 'rgba(127, 0, 127, 0.5)' }, +] as const </script> <template> <Story group="compounds" title="ColorPicker/Full Demo" :layout="{ type: 'grid', width: '50%' }" > - <Variant title="Complete Color Picker"> - <ColorPicker default-value="#7f007f" /> - </Variant> - - <Variant title="Red Color"> - <ColorPicker default-value="#ff0000" /> - </Variant> - - <Variant title="Green Color"> - <ColorPicker default-value="#00ff00" /> - </Variant> - - <Variant title="Blue Color"> - <ColorPicker default-value="#0000ff" /> - </Variant> - - <Variant title="Semi-transparent"> - <ColorPicker default-value="rgba(127, 0, 127, 0.5)" /> + <Variant v-for="variant in variants" :key="variant.title" :title="variant.title"> + <ColorPicker :default-value="variant.value" /> </Variant> </Story> </template>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/ColorPicker/story/ColorPickerDemo.story.vue` around lines 11 - 29, The story repeats multiple Variant blocks with ColorPicker; replace these duplicated blocks by creating a small array of variant configs (e.g., titles and defaultValue strings) and render the variants with a loop (use the Variant component and ColorPicker inside a v-for) so each entry drives a Variant title and the ColorPicker default-value prop (refer to Variant and ColorPicker in ColorPickerDemo.story.vue to locate where to change).packages/core/src/ColorSwatchPicker/story/ColorSwatchPickerDemo.story.vue (1)
29-40: Consider extracting the repeated swatch-item subtree.The same item/swatch/indicator markup is repeated five times, which increases maintenance cost for future style/behavior tweaks. A small local reusable render block/component would keep variants focused on state differences only.
Also applies to: 50-61, 76-87, 100-111, 120-131
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/ColorSwatchPicker/story/ColorSwatchPickerDemo.story.vue` around lines 29 - 40, Extract the repeated item subtree into a small local reusable component or render function (e.g., SwatchItem) and replace each repeated block with that component to avoid duplication; locate the markup using ColorSwatchPickerItem, ColorSwatchPickerItemSwatch, and ColorSwatchPickerItemIndicator inside ColorSwatchPickerRoot and move the inner template (swatch + indicator + props :value and :key) into the new SwatchItem that accepts the color (or v-bind props) and reuses the same classes/peer attributes, then update each v-for usage (the places around the colors array iterations) to render <SwatchItem :color="color" /> instead of duplicating the three-node subtree.packages/core/src/ColorSwatch/ColorSwatch.test.ts (1)
8-14: Unmount the body-attached wrapper in the accessibility test.The wrapper in this test is attached to
document.bodyand should be explicitly unmounted to avoid cross-test residue.Suggested fix
it('should pass axe accessibility tests', async () => { const wrapper = mount(ColorSwatch, { props: { color: '#E5484D' }, attachTo: document.body, }) - expect(await axe(wrapper.element)).toHaveNoViolations() + try { + expect(await axe(wrapper.element)).toHaveNoViolations() + } + finally { + wrapper.unmount() + } })🤖 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 8 - 14, The test in ColorSwatch.test.ts mounts ColorSwatch to document.body but never unmounts it, which can leak DOM between tests; update the it('should pass axe accessibility tests') block to ensure the mounted wrapper (the variable named wrapper returned from mount(ColorSwatch, ...)) is unmounted afterwards—e.g., run the axe assertion then call wrapper.unmount() (or wrap the axe call in try/finally and call wrapper.unmount() in finally) so the DOM node attached to document.body is always cleaned up.packages/core/src/ColorField/ColorFieldInput.vue (1)
123-123:aria-invalidbinding is currently a no-op.Line 123 always resolves to
undefined; either bind real invalid state or remove this attribute binding.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/ColorField/ColorFieldInput.vue` at line 123, The aria-invalid binding in ColorFieldInput.vue is a no-op (always undefined); replace it with a real invalid state or remove it. Either bind to an actual validation value (e.g. :aria-invalid="rootContext.isInvalid" or :aria-invalid="rootContext.channel?.value ? undefined : true") or compute a clear boolean/computed like isInvalid and use :aria-invalid="isInvalid" so the attribute reflects actual validity; update or add the computed/prop accordingly in the component.packages/core/src/shared/color/channel.ts (2)
241-412: Avoid duplicating conversion math across modules.These helper implementations mirror
packages/core/src/shared/color/convert.tsand are likely to drift over time. Consider extracting shared internal conversion primitives into a single module to remove duplicate logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/shared/color/channel.ts` around lines 241 - 412, The rgbToHsl, hslToRgb, rgbToHsb and hsbToRgb helper functions duplicate conversion math found elsewhere; extract these conversion primitives into a single internal module (e.g., a shared convert/primitives module) and have channel.ts import and call those functions instead of reimplementing them; update channel.ts to replace the local implementations of rgbToHsl, hslToRgb, rgbToHsb and hsbToRgb with imports from the shared module and remove the duplicated code to ensure a single source of truth (also update any existing convert.ts to re-export or delegate to the new primitives as needed).
179-186: Remove unreachable single-channel conversion-back block.This branch is dead because
channels.length === 1already returns at Line 126–Line 128.Suggested cleanup
- // For color areas, keep the working color space to preserve precision - // Only convert back if we're in single-channel mode (channels.length === 1) - // In multi-channel mode (like 2D color area), the working color space is more appropriate - if (channels.length === 1 && workingColor.space !== color.space) { - if (color.space === 'rgb') - return convertToRgb(workingColor) - if (color.space === 'hsl') - return convertToHsl(workingColor) - if (color.space === 'hsb') - return convertToHsb(workingColor) - } - return workingColor🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/shared/color/channel.ts` around lines 179 - 186, The single-channel conversion-back block guarded by "if (channels.length === 1 && workingColor.space !== color.space)" is unreachable because channels.length === 1 is already handled earlier; remove this entire conditional and its calls to convertToRgb, convertToHsl, and convertToHsb (the block referencing workingColor and color.space) to clean up dead code and avoid redundant logic in channel processing.packages/core/src/ColorArea/ColorArea.test.ts (1)
18-21: Helper can avoid runtime regex construction.This parser can be simplified to string/token parsing to reduce brittleness and avoid dynamic regex construction in tests.
🧪 Suggested helper refactor
function getYValue(wrapper: VueWrapper, channelName: string): number { const text = wrapper.find('[role="slider"]').attributes('aria-valuetext') ?? '' - const match = text.match(new RegExp(`${channelName} (\\d+)`)) - return match ? Number(match[1]) : NaN + const token = `${channelName} ` + const start = text.indexOf(token) + if (start === -1) return NaN + const raw = text.slice(start + token.length).split(/[, ]/)[0] + const value = Number(raw) + return Number.isNaN(value) ? NaN : value }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/ColorArea/ColorArea.test.ts` around lines 18 - 21, The getYValue helper constructs a RegExp at runtime to parse aria-valuetext; replace it with deterministic string/token parsing in getYValue(wrapper: VueWrapper, channelName: string) by reading wrapper.find('[role="slider"]').attributes('aria-valuetext'), splitting or tokenizing the string (e.g. by whitespace or ":"), locating the token that equals channelName or directly checking startsWith(`${channelName} `), and then parseInt the following token to return the numeric value (or NaN on failure). This removes dynamic RegExp construction and makes parsing more robust.packages/core/src/ColorSlider/ColorSlider.test.ts (1)
3-16: Global DOM mocks should be scoped and restored.These prototype overrides are applied globally and never restored, which can leak state across unrelated test files.
🧹 Suggested setup/teardown pattern
-import { beforeEach, describe, expect, it, vi } from 'vitest' +import { afterAll, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest' @@ -globalThis.ResizeObserver = class ResizeObserver { - observe() {} - unobserve() {} - disconnect() {} -} -window.HTMLElement.prototype.scrollIntoView = vi.fn() -window.HTMLElement.prototype.hasPointerCapture = vi.fn().mockImplementation(id => id) -window.HTMLElement.prototype.releasePointerCapture = vi.fn() -window.HTMLElement.prototype.setPointerCapture = vi.fn() +const OriginalResizeObserver = globalThis.ResizeObserver +const originalScrollIntoView = window.HTMLElement.prototype.scrollIntoView +const originalHasPointerCapture = window.HTMLElement.prototype.hasPointerCapture +const originalReleasePointerCapture = window.HTMLElement.prototype.releasePointerCapture +const originalSetPointerCapture = window.HTMLElement.prototype.setPointerCapture + +beforeAll(() => { + globalThis.ResizeObserver = class ResizeObserver { + observe() {} + unobserve() {} + disconnect() {} + } + window.HTMLElement.prototype.scrollIntoView = vi.fn() + window.HTMLElement.prototype.hasPointerCapture = vi.fn().mockReturnValue(false) + window.HTMLElement.prototype.releasePointerCapture = vi.fn() + window.HTMLElement.prototype.setPointerCapture = vi.fn() +}) + +afterAll(() => { + globalThis.ResizeObserver = OriginalResizeObserver + window.HTMLElement.prototype.scrollIntoView = originalScrollIntoView + window.HTMLElement.prototype.hasPointerCapture = originalHasPointerCapture + window.HTMLElement.prototype.releasePointerCapture = originalReleasePointerCapture + window.HTMLElement.prototype.setPointerCapture = originalSetPointerCapture +})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/ColorSlider/ColorSlider.test.ts` around lines 3 - 16, The test file is applying global DOM prototype overrides (ResizeObserver and HTMLElement methods scrollIntoView, hasPointerCapture, releasePointerCapture, setPointerCapture) without restoring them, risking test pollution; fix by saving the original globals/prototypes at top of the file, set the test doubles inside a beforeEach (or beforeAll if appropriate) and restore the originals in an afterEach (or afterAll) teardown so ResizeObserver and the four HTMLElement prototype methods are replaced only for the test scope and reliably restored after each test run (reference ResizeObserver and window.HTMLElement.prototype.scrollIntoView / hasPointerCapture / releasePointerCapture / setPointerCapture to locate the changes).packages/core/src/ColorField/story/ColorFieldDemo.story.vue (1)
1-4: “Controlled” demo is effectively read-only.Using
model-valuewithout an update path means edits won’t persist, so this variant does not actually demonstrate controlled behavior.♻️ Proposed fix
<script setup lang="ts"> +import { ref } from 'vue' import { ColorFieldInput, ColorFieldRoot } from '../' import ColorField from './_ColorField.vue' + +const controlledColor = ref('#3b82f6') </script> @@ <ColorFieldRoot - model-value="#3b82f6" + v-model="controlledColor" class="inline-flex flex-col gap-1" >Also applies to: 112-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/ColorField/story/ColorFieldDemo.story.vue` around lines 1 - 4, The "Controlled" story currently passes a static model-value so edits appear read-only; change the story to own a reactive value and forward updates from the components (use v-model or listen for update:model-value and set the ref) so the ColorFieldInput/ColorFieldRoot (and the ColorField demo at the other block around lines 112-123) mutate the story's state; specifically, create a ref (e.g., colorValue) in the script setup and bind it to ColorFieldInput/ColorFieldRoot/ColorField with v-model or onUpdate:modelValue to persist edits.
🤖 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/ColorArea/ColorArea.test.ts`:
- Around line 264-266: The test currently asserts an invalid empty-string ARIA
value; update the spec in ColorArea.test.ts (the it block expecting
aria-disabled on the element with role="application") to expect "true" when
disabled, and update the component binding in ColorAreaArea.vue (the
:aria-disabled expression that currently uses disabled ? '' : undefined) to use
"true" / "false" (i.e., :aria-disabled="disabled ? 'true' : 'false'") so the
runtime and test both use ARIA-compliant values.
In `@packages/core/src/ColorArea/ColorAreaArea.vue`:
- Around line 79-104: The keyboard navigation uses the X-axis step for vertical
moves causing incorrect scaling; obtain the Y-axis step from
rootContext.yRange.value (e.g., const { step: yStep } =
rootContext.yRange.value), compute a yStepSize using the same stepMultiplier
logic, and use yStepSize for yDelta assignments in the switch handling event.key
(including 'ArrowUp', 'ArrowDown', 'PageUp', 'PageDown'); also apply the same
change in the similar block around the yDelta assignments at lines 118-121 so
vertical movement uses the Y channel's step instead of the X step.
- Around line 44-45: Replace uses of event.target with event.currentTarget when
calling setPointerCapture/releasePointerCapture in the ColorAreaArea.vue pointer
handlers so the capture is attached to the area element rather than a nested
child; update the local variable to const target = event.currentTarget as
HTMLElement (or add a null check if currentTarget can be null) wherever you call
target.setPointerCapture(event.pointerId) and
target.releasePointerCapture(event.pointerId) (the occurrences around the
existing setPointerCapture/releasePointerCapture calls noted in the diff).
- Around line 126-134: The Primitive element (ref="primitiveElement") needs to
be focusable so the `@keydown` handler on handleKeyDown can be triggered; update
the props passed to Primitive to include a tabindex that is 0 when enabled and
-1 when rootContext.disabled.value is true (or otherwise ensure focusability
when asChild is used by forwarding tabindex), so keyboard users can tab into the
ColorArea and disabled state prevents focus.
In `@packages/core/src/ColorArea/ColorAreaRoot.vue`:
- Around line 185-191: The current logic always appends a preserved hue update
in updateValues which overwrites the dragged hue when hue is the active axis;
modify the hue-preservation block (the conditional that checks colorSpace.value
=== 'hsl' or 'hsb' and pushes { channel: 'hue', value: hueValue.value }) to
first check whether hue is already the active channel (e.g., xChannel === 'hue'
or yChannel === 'hue' or the component's active axis flag) and only push the hue
entry when hue is not an active axis, leaving user drag updates intact.
- Around line 193-203: The component only emits 'changeEnd' in commitValues but
never emits 'change' during live updates, so listeners don't get updates while
dragging; after updating color.value in the live-update path (the code that
calls color.value = setChannelValues(color.value, channels) and toggles
isUpdating via nextTick), add a live emit: call emits('change',
colorToString(color.value, 'hex')) immediately after setting color.value (before
or just after nextTick) so consumers receive intermediate color updates; keep
commitValues unchanged to continue emitting 'changeEnd' on finalize.
In `@packages/core/src/ColorArea/ColorAreaThumb.vue`:
- Around line 50-58: The thumb currently removes tabindex when
rootContext.disabled.value is true but does not set aria-disabled; update
ColorAreaThumb.vue to add an aria-disabled attribute bound to the disabled state
(e.g., use rootContext.disabled.value to output "true" when disabled and
undefined otherwise) so the slider role announces its disabled state; locate the
template attributes around :tabindex, aria-roledescription, and :data-disabled
and add :aria-disabled="rootContext.disabled.value ? 'true' : undefined" (or
equivalent) to the element that defines the slider/thumb.
In `@packages/core/src/ColorArea/utils.ts`:
- Around line 13-17: The convertValueToPercentage function can divide by zero
when max === min; add a guard at the start of convertValueToPercentage
(referencing the function name) to handle maxSteps === 0 (or max === min) and
return a sensible default (e.g., 0 or clamp(value) result) instead of performing
the division, then proceed with the existing percentPerStep calculation and
clamp(percentage, 0, 100); this avoids the division-by-zero and preserves the
clamp behavior.
In `@packages/core/src/ColorField/ColorField.test.ts`:
- Around line 32-46: The two tests use a permissive
expect(wrapper.emitted()).toBeDefined(); replace these with concrete assertions:
after setting the input value and blurring, assert the specific event your
component emits (e.g., 'update:modelValue' or the component's emit name) was
emitted once and that its payload equals the hex string '#00ff00' by checking
wrapper.emitted()[eventName].length and wrapper.emitted()[eventName][0][0]; do
this in both tests (the "should update value on input" and "should accept valid
hex colors" cases) so you verify the exact event and payload rather than just
any emission.
In `@packages/core/src/ColorField/ColorFieldInput.vue`:
- Around line 89-107: The current pre-validation in ColorFieldInput.vue is too
permissive because it tests only the inserted character and then uses
parseFloat, allowing malformed strings like multiple dots or minuses; update the
logic in the input handler (the block that computes nextValue from target, data,
selectionStart/End and currently uses parseFloat) to validate the entire
nextValue against a strict numeric regex such as /^-?\d*\.?\d*$/ (or
/^-?(?:\d+|\d*\.\d+)$/ if you want at least one digit), and reject the event
(event.preventDefault()) when nextValue fails that full-string regex; keep the
early-returns for single-character in-progress values only if they still match
the stricter pattern, and remove reliance on isNaN(parseFloat(nextValue)) for
this pre-validation.
In `@packages/core/src/ColorField/ColorFieldRoot.vue`:
- Around line 148-150: updateValue currently only assigns inputValue.value and
never toggles the editing guard; modify the updateValue function to set
isEditing.value = true when called (and ensure isEditing is a Ref<boolean>),
e.g., inside updateValue set isEditing.value = true before assigning
inputValue.value; also ensure elsewhere (e.g., onBlur or save handlers)
isEditing.value is reset to false so the editing guard behaves correctly.
- Around line 177-188: The addHexValue function can produce invalid hex strings
when delta (step) is non-integer because hexInt + delta may be fractional before
toString(16); fix it by ensuring the value converted to hex is an integer:
compute clamped as you do, then round (e.g. Math.round) the clamped result to an
integer before calling toString(16), then build the hex string and set
color.value via parseColor and update inputValue; reference addHexValue,
color.value, clamped, parseColor and inputValue to locate the change.
In `@packages/core/src/ColorSlider/ColorSliderRoot.vue`:
- Around line 88-105: The toNativeSpace function forces 'saturation' to HSL
math, ignoring colorSpace; update its signature to accept the current colorSpace
(e.g., add a parameter colorSpace: 'hsl'|'hsb'|'rgb' or use existing scoped
colorSpace), then change the 'saturation' case to return convertToHsb(color)
when colorSpace === 'hsb' and convertToHsl(color) otherwise; update all callers
of toNativeSpace (the locations referenced around the existing calls) to pass
the current colorSpace so saturation uses the correct convertToHsb/convertToHsl
branch; keep other cases using convertToRgb, convertToHsb, convertToHsl, and
alpha unchanged.
In `@packages/core/src/ColorSlider/utils.ts`:
- Around line 25-29: The convertValueToPercentage function can produce
NaN/Infinity when max === min (maxSteps === 0); guard that case by detecting
zero-width ranges in convertValueToPercentage and return a safe default (e.g.,
0) instead of performing the division, then continue to clamp the result as
before; update the function so it checks maxSteps (or compares max and min)
before computing percentPerStep to avoid divide-by-zero and ensure downstream
style/position logic using convertValueToPercentage and clamp gets a valid
number.
In `@packages/core/src/shared/color/channel.ts`:
- Around line 57-60: The read path correctly uses (color as HSBColor).s when
color.space === 'hsb', but the single-channel write path always edits HSL
saturation causing broken round-trips; update the write logic to branch on
color.space and set the saturation on (color as HSBColor).s when color.space ===
'hsb' and on (color as HSLColor).s when color.space === 'hsl', and apply the
same fix to the other write site referenced (the block around the second
occurrence for saturation handling). Ensure you use the same type assertions
(HSBColor/HSLColor) and property 's' as in the read path so reads and writes are
symmetric.
In `@packages/core/src/shared/color/gradient.ts`:
- Around line 86-104: The red/green/blue gradient helpers (getRedGradient,
getGreenGradient, getBlueGradient) currently fall back to fixed 128 channel
values for non-RGB inputs; change them to derive actual r/g/b/alpha by
converting the incoming Color to the RGB space instead of using {128,128,1}.
Update each function to, when color.space !== 'rgb', call your project’s color
conversion utility (e.g., convertToRgb or an equivalent function you have) to
obtain {r,g,b,alpha} and then build start/end with colorToString as before so
gradients reflect the true converted channels.
In `@packages/core/src/shared/color/parse.ts`:
- Line 104: The regex in parseHsb only matches "hsb" variants and thus rejects
"hsv"/"hsva"; update the pattern used in parseHsb (replace the current
/hsb[av]?\(\s*([\d.]+)\s*,\s*([\d.]+)%?\s*,\s*([\d.]+)%?\s*(?:,\s*([\d.]+)\s*)?\)/
with a variant that accepts both "hsb" and "hsv", e.g. use
/h(?:sb|sv)[av]?\(\s*([\d.]+)\s*,\s*([\d.]+)%?\s*,\s*([\d.]+)%?\s*(?:,\s*([\d.]+)\s*)?\)/
so parseHsb will match hsv/hsva as well while keeping the same capture groups
and behavior.
---
Minor comments:
In `@packages/core/src/ColorField/story/_ColorField.vue`:
- Line 20: The local ref color is initialized from props.defaultValue once
(const color = ref(props.defaultValue ?? '#ff0000')), so updates to the prop
(e.g., Storybook controls) won't propagate; change this by syncing the ref with
the prop—either replace the standalone ref with a computed getter/setter that
reads/writes props.defaultValue or add a watch on props.defaultValue that
updates color when the prop changes; reference the existing symbols color,
props.defaultValue, and the use of ref to locate and update the implementation.
In `@packages/core/src/ColorSlider/ColorSlider.test.ts`:
- Around line 197-212: The test only checks min/max but not that keyboard
movement respects the step prop; update the 'custom step prop' test in
ColorSlider.test.ts to simulate keyboard interaction on the slider thumb (the
element found by wrapper.find('[role="slider"]')) after mounting ColorSlider
with step: 10 and initial value '#ff0000', focus the thumb, trigger a relevant
keydown (e.g., 'ArrowRight' or 'ArrowUp' depending on the hue channel) and
assert that thumb.attributes('aria-valuenow') changed from the initial value to
initial+10 (e.g., '0' -> '10'), ensuring the SliderRoot/ColorSlider keyboard
handling uses the provided step prop.
- Line 13: The mock for window.HTMLElement.prototype.hasPointerCapture returns a
number (id) which violates the DOM contract expecting a boolean; update the mock
implementation in ColorSlider.test (the hasPointerCapture mock) to return a
boolean instead (e.g., return !!id or a fixed true/false) so tests reflect the
correct return type for HTMLElement.hasPointerCapture.
In `@packages/core/src/ColorSwatch/ColorSwatch.vue`:
- Around line 64-74: The computed colorContrast should skip calling
getColorContrast when the swatch represents "no color" to avoid DEV warnings;
update the colorContrast computed (which reads colorString.value and calls
getColorContrast) to first check for empty/falsy values and explicit transparent
states (e.g., empty string or 'transparent', and any other project-specific
no-color sentinel) and immediately return undefined in those cases, otherwise
call getColorContrast inside the try/catch as before.
In `@packages/core/src/shared/color/gradient.ts`:
- Around line 147-151: In the 'brightness' branch inside gradient.ts (case
'brightness') the fallback gradient is currently returned as
linear-gradient(${direction}, rgba(0,0,0,0), rgba(0,0,0,1)) which places black
at the top for axis === 'y'; update the logic so the RGBA stop order is swapped
when axis is 'y' (or compute a corrected direction) so that for vertical
gradients the transparent stop comes first at the top and the black stop is at
the bottom; modify the code around the 'direction' usage in the case
'brightness' block to conditionally return either linear-gradient(${direction},
rgba(0,0,0,0), rgba(0,0,0,1)) or linear-gradient(${direction}, rgba(0,0,0,1),
rgba(0,0,0,0)) depending on axis === 'y'.
In `@packages/core/src/shared/color/parse.ts`:
- Around line 74-75: The regexes that parse rgb/rgba (the one assigning match
from rgb.match(/rgba?\(.../)) and the two other similar regexes later in this
file are too permissive and accept trailing characters (e.g., "rgb(1,2,3)foo");
update each of those regex literals to be anchored with ^ at the start and $ at
the end so they only match the entire input string (modify the regex passed to
String.match in the rgb/rgba parsing expression and the two analogous match
expressions later in the file).
---
Nitpick comments:
In `@packages/core/src/ColorArea/ColorArea.test.ts`:
- Around line 18-21: The getYValue helper constructs a RegExp at runtime to
parse aria-valuetext; replace it with deterministic string/token parsing in
getYValue(wrapper: VueWrapper, channelName: string) by reading
wrapper.find('[role="slider"]').attributes('aria-valuetext'), splitting or
tokenizing the string (e.g. by whitespace or ":"), locating the token that
equals channelName or directly checking startsWith(`${channelName} `), and then
parseInt the following token to return the numeric value (or NaN on failure).
This removes dynamic RegExp construction and makes parsing more robust.
In `@packages/core/src/ColorField/ColorFieldInput.vue`:
- Line 123: The aria-invalid binding in ColorFieldInput.vue is a no-op (always
undefined); replace it with a real invalid state or remove it. Either bind to an
actual validation value (e.g. :aria-invalid="rootContext.isInvalid" or
:aria-invalid="rootContext.channel?.value ? undefined : true") or compute a
clear boolean/computed like isInvalid and use :aria-invalid="isInvalid" so the
attribute reflects actual validity; update or add the computed/prop accordingly
in the component.
In `@packages/core/src/ColorField/story/ColorFieldDemo.story.vue`:
- Around line 1-4: The "Controlled" story currently passes a static model-value
so edits appear read-only; change the story to own a reactive value and forward
updates from the components (use v-model or listen for update:model-value and
set the ref) so the ColorFieldInput/ColorFieldRoot (and the ColorField demo at
the other block around lines 112-123) mutate the story's state; specifically,
create a ref (e.g., colorValue) in the script setup and bind it to
ColorFieldInput/ColorFieldRoot/ColorField with v-model or onUpdate:modelValue to
persist edits.
In `@packages/core/src/ColorPicker/story/ColorPickerDemo.story.vue`:
- Around line 11-29: The story repeats multiple Variant blocks with ColorPicker;
replace these duplicated blocks by creating a small array of variant configs
(e.g., titles and defaultValue strings) and render the variants with a loop (use
the Variant component and ColorPicker inside a v-for) so each entry drives a
Variant title and the ColorPicker default-value prop (refer to Variant and
ColorPicker in ColorPickerDemo.story.vue to locate where to change).
In `@packages/core/src/ColorSlider/ColorSlider.test.ts`:
- Around line 3-16: The test file is applying global DOM prototype overrides
(ResizeObserver and HTMLElement methods scrollIntoView, hasPointerCapture,
releasePointerCapture, setPointerCapture) without restoring them, risking test
pollution; fix by saving the original globals/prototypes at top of the file, set
the test doubles inside a beforeEach (or beforeAll if appropriate) and restore
the originals in an afterEach (or afterAll) teardown so ResizeObserver and the
four HTMLElement prototype methods are replaced only for the test scope and
reliably restored after each test run (reference ResizeObserver and
window.HTMLElement.prototype.scrollIntoView / hasPointerCapture /
releasePointerCapture / setPointerCapture to locate the changes).
In `@packages/core/src/ColorSwatch/ColorSwatch.test.ts`:
- Around line 8-14: The test in ColorSwatch.test.ts mounts ColorSwatch to
document.body but never unmounts it, which can leak DOM between tests; update
the it('should pass axe accessibility tests') block to ensure the mounted
wrapper (the variable named wrapper returned from mount(ColorSwatch, ...)) is
unmounted afterwards—e.g., run the axe assertion then call wrapper.unmount() (or
wrap the axe call in try/finally and call wrapper.unmount() in finally) so the
DOM node attached to document.body is always cleaned up.
In `@packages/core/src/ColorSwatchPicker/story/ColorSwatchPickerDemo.story.vue`:
- Around line 29-40: Extract the repeated item subtree into a small local
reusable component or render function (e.g., SwatchItem) and replace each
repeated block with that component to avoid duplication; locate the markup using
ColorSwatchPickerItem, ColorSwatchPickerItemSwatch, and
ColorSwatchPickerItemIndicator inside ColorSwatchPickerRoot and move the inner
template (swatch + indicator + props :value and :key) into the new SwatchItem
that accepts the color (or v-bind props) and reuses the same classes/peer
attributes, then update each v-for usage (the places around the colors array
iterations) to render <SwatchItem :color="color" /> instead of duplicating the
three-node subtree.
In `@packages/core/src/shared/color/channel.ts`:
- Around line 241-412: The rgbToHsl, hslToRgb, rgbToHsb and hsbToRgb helper
functions duplicate conversion math found elsewhere; extract these conversion
primitives into a single internal module (e.g., a shared convert/primitives
module) and have channel.ts import and call those functions instead of
reimplementing them; update channel.ts to replace the local implementations of
rgbToHsl, hslToRgb, rgbToHsb and hsbToRgb with imports from the shared module
and remove the duplicated code to ensure a single source of truth (also update
any existing convert.ts to re-export or delegate to the new primitives as
needed).
- Around line 179-186: The single-channel conversion-back block guarded by "if
(channels.length === 1 && workingColor.space !== color.space)" is unreachable
because channels.length === 1 is already handled earlier; remove this entire
conditional and its calls to convertToRgb, convertToHsl, and convertToHsb (the
block referencing workingColor and color.space) to clean up dead code and avoid
redundant logic in channel processing.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (44)
.histoire/vite.config.tspackages/core/constant/components.tspackages/core/src/ColorArea/ColorArea.test.tspackages/core/src/ColorArea/ColorAreaArea.vuepackages/core/src/ColorArea/ColorAreaRoot.vuepackages/core/src/ColorArea/ColorAreaThumb.vuepackages/core/src/ColorArea/index.tspackages/core/src/ColorArea/story/ColorAreaDemo.story.vuepackages/core/src/ColorArea/story/_ColorArea.vuepackages/core/src/ColorArea/utils.tspackages/core/src/ColorField/ColorField.test.tspackages/core/src/ColorField/ColorFieldInput.vuepackages/core/src/ColorField/ColorFieldRoot.vuepackages/core/src/ColorField/index.tspackages/core/src/ColorField/story/ColorFieldDemo.story.vuepackages/core/src/ColorField/story/_ColorField.vuepackages/core/src/ColorPicker/story/ColorPickerDemo.story.vuepackages/core/src/ColorPicker/story/_ColorPicker.vuepackages/core/src/ColorSlider/ColorSlider.test.tspackages/core/src/ColorSlider/ColorSliderRoot.vuepackages/core/src/ColorSlider/ColorSliderThumb.vuepackages/core/src/ColorSlider/ColorSliderTrack.vuepackages/core/src/ColorSlider/index.tspackages/core/src/ColorSlider/story/ColorSliderDemo.story.vuepackages/core/src/ColorSlider/story/_ColorSlider.vuepackages/core/src/ColorSlider/story/_ColorSliderCompound.vuepackages/core/src/ColorSlider/utils.tspackages/core/src/ColorSwatch/ColorSwatch.test.tspackages/core/src/ColorSwatch/ColorSwatch.vuepackages/core/src/ColorSwatch/story/ColorSwatchDemo.story.vuepackages/core/src/ColorSwatch/story/_ColorSwatch.vuepackages/core/src/ColorSwatchPicker/ColorSwatchPicker.test.tspackages/core/src/ColorSwatchPicker/ColorSwatchPickerItem.vuepackages/core/src/ColorSwatchPicker/story/ColorSwatchPickerDemo.story.vuepackages/core/src/ColorSwatchPicker/story/_ColorSwatchPicker.vuepackages/core/src/index.tspackages/core/src/shared/color/channel.tspackages/core/src/shared/color/convert.tspackages/core/src/shared/color/gradient.test.tspackages/core/src/shared/color/gradient.tspackages/core/src/shared/color/index.tspackages/core/src/shared/color/parse.tspackages/core/src/shared/color/types.tspackages/core/src/shared/color/utils.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/core/src/shared/color/utils.test.ts
- packages/core/src/ColorSwatchPicker/story/_ColorSwatchPicker.vue
- packages/core/src/ColorSwatch/story/_ColorSwatch.vue
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
packages/core/src/ColorArea/ColorAreaArea.vue (2)
124-136:⚠️ Potential issue | 🟠 MajorMake the area focusable so keyboard interaction is reachable.
The keydown handler won't be usable on a plain
divunless it can receive focus. Add atabindexattribute.🔧 Suggested fix
<Primitive ref="primitiveElement" :as-child="asChild" :as="as" + :tabindex="rootContext.disabled.value ? -1 : 0" role="application"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/ColorArea/ColorAreaArea.vue` around lines 124 - 136, The Primitive element isn't focusable so handleKeyDown won't run; add a tabindex binding on the Primitive (near ref="primitiveElement") to make it focusable when not disabled (e.g., set tabindex to 0 when enabled and -1 when rootContext.disabled.value is true) so keyboard interaction via handleKeyDown works and focus is prevented when disabled.
44-45:⚠️ Potential issue | 🟠 MajorUse
event.currentTargetfor pointer capture lifecycle.Using
event.targetcan attach capture to a nested child instead of the area element, making drag state brittle. This applies to all three pointer handlers.🔧 Suggested fix
function handlePointerDown(event: PointerEvent) { if (rootContext.disabled.value) return - const target = event.target as HTMLElement + const target = event.currentTarget as HTMLElement target.setPointerCapture(event.pointerId)Apply similar changes to
handlePointerMove(line 57) andhandlePointerUp(line 68).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/ColorArea/ColorAreaArea.vue` around lines 44 - 45, The pointer handlers currently call setPointerCapture/releasePointerCapture on event.target which can be a nested child; change them to use event.currentTarget as HTMLElement instead so capture is attached to the area element consistently—update handlePointerDown (replace target = event.target with currentTarget = event.currentTarget and call currentTarget.setPointerCapture(event.pointerId)), and make the analogous changes in handlePointerMove and handlePointerUp (use event.currentTarget and call releasePointerCapture where appropriate) to ensure the pointer capture lifecycle is tied to the area element.
🧹 Nitpick comments (1)
docs/content/meta/ColorAreaRoot.md (1)
72-83: Consider adding descriptions for emitted events.The
update:colorandupdate:modelValueevents have empty descriptions. While this is auto-generated content, adding brief descriptions would improve API discoverability for users.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/meta/ColorAreaRoot.md` around lines 72 - 83, Add brief descriptions for the emitted events in the EmitsTable component: for 'update:color' (type '[value: Color]') state that it emits the selected Color object when the color area changes, and for 'update:modelValue' (type '[value: string]') state that it emits the color value string (e.g., HEX/RGB) for v-model binding; update the description fields next to the 'name' entries so the table shows these short explanatory texts.
🤖 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/components/demo/ColorField/tailwind/index.vue`:
- Around line 17-27: The label element is not programmatically connected to the
input which hurts accessibility; update the markup so the label is associated
with the ColorFieldInput by either giving the ColorFieldInput an id (e.g.,
id="color-field") and adding for="color-field" to the <label>, or by wrapping
the ColorFieldInput inside the <label> element; modify the component/template
where ColorFieldInput is rendered to accept/propagate the id attribute if needed
so the label and input are linked.
In `@docs/content/docs/components/color-slider.md`:
- Around line 51-55: The anatomy snippet nests ColorSliderThumb inside
ColorSliderTrack, which is inconsistent with examples; update the snippet so
ColorSliderRoot directly contains ColorSliderTrack and ColorSliderThumb as
siblings (i.e., place ColorSliderThumb alongside ColorSliderTrack under
ColorSliderRoot) using the same canonical structure as other examples that
reference ColorSliderRoot, ColorSliderTrack, and ColorSliderThumb.
In `@docs/content/meta/ColorSliderRoot.md`:
- Around line 90-101: The EmitsTable in ColorSliderRoot.md is missing the
runtime emits 'change' and 'changeEnd' defined in ColorSliderRootEmits in
packages/core/src/ColorSlider/ColorSliderRoot.vue; update the EmisTable data
array to include entries for 'change' and 'changeEnd' (with appropriate
description and types matching the component, e.g., '[value: Color]' or similar)
so the docs match the component's emitted events.
In `@packages/core/src/ColorArea/ColorAreaArea.vue`:
- Around line 130-131: Change the ARIA attribute to use a proper ARIA boolean
string: replace the current binding for aria-disabled so it emits "true" when
the control is disabled and omits the attribute when not (e.g., in
ColorAreaArea.vue update :aria-disabled to rootContext.disabled.value ? 'true' :
undefined), keep or adjust :data-disabled as needed but ensure aria-disabled
uses the literal "true" string instead of an empty string; reference the
rootContext.disabled.value expression and the aria-disabled attribute in your
fix.
In `@packages/core/src/ColorSlider/ColorSliderRoot.vue`:
- Around line 8-34: The prop ColorSliderRootProps.modelValue allows string |
Color but ColorSliderRootEmits currently types 'update:modelValue' as string
only; update the emits type to 'update:modelValue': [value: string | Color] and
adjust the component's v-model setter (the function that emits
'update:modelValue') to preserve the original input type (if the new value is a
Color emit the Color, if it's a hex/string emit the string) rather than always
converting to a hex string; ensure any internal calls that emit
'update:modelValue' use the same value shape so consumers binding Ref<Color>
remain type-safe.
---
Duplicate comments:
In `@packages/core/src/ColorArea/ColorAreaArea.vue`:
- Around line 124-136: The Primitive element isn't focusable so handleKeyDown
won't run; add a tabindex binding on the Primitive (near ref="primitiveElement")
to make it focusable when not disabled (e.g., set tabindex to 0 when enabled and
-1 when rootContext.disabled.value is true) so keyboard interaction via
handleKeyDown works and focus is prevented when disabled.
- Around line 44-45: The pointer handlers currently call
setPointerCapture/releasePointerCapture on event.target which can be a nested
child; change them to use event.currentTarget as HTMLElement instead so capture
is attached to the area element consistently—update handlePointerDown (replace
target = event.target with currentTarget = event.currentTarget and call
currentTarget.setPointerCapture(event.pointerId)), and make the analogous
changes in handlePointerMove and handlePointerUp (use event.currentTarget and
call releasePointerCapture where appropriate) to ensure the pointer capture
lifecycle is tied to the area element.
---
Nitpick comments:
In `@docs/content/meta/ColorAreaRoot.md`:
- Around line 72-83: Add brief descriptions for the emitted events in the
EmitsTable component: for 'update:color' (type '[value: Color]') state that it
emits the selected Color object when the color area changes, and for
'update:modelValue' (type '[value: string]') state that it emits the color value
string (e.g., HEX/RGB) for v-model binding; update the description fields next
to the 'name' entries so the table shows these short explanatory texts.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
docs/.vitepress/config.tsdocs/components/demo/ColorArea/tailwind/index.vuedocs/components/demo/ColorField/tailwind/index.vuedocs/components/demo/ColorSlider/tailwind/index.vuedocs/components/examples/ColorPicker/index.vuedocs/content/docs/components/color-area.mddocs/content/docs/components/color-field.mddocs/content/docs/components/color-slider.mddocs/content/examples/color-picker.mddocs/content/meta/ColorAreaArea.mddocs/content/meta/ColorAreaRoot.mddocs/content/meta/ColorAreaThumb.mddocs/content/meta/ColorFieldInput.mddocs/content/meta/ColorFieldRoot.mddocs/content/meta/ColorSliderRoot.mddocs/content/meta/ColorSliderThumb.mddocs/content/meta/ColorSliderTrack.mdpackages/core/src/ColorArea/ColorAreaArea.vuepackages/core/src/ColorArea/utils.tspackages/core/src/ColorField/ColorFieldRoot.vuepackages/core/src/ColorSlider/ColorSliderRoot.vuepackages/core/src/ColorSlider/utils.tspackages/core/src/shared/color/channel.ts
✅ Files skipped from review due to trivial changes (5)
- docs/content/docs/components/color-area.md
- docs/content/meta/ColorAreaThumb.md
- docs/content/meta/ColorAreaArea.md
- docs/content/docs/components/color-field.md
- docs/content/meta/ColorFieldRoot.md
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/core/src/ColorArea/utils.ts
- docs/.vitepress/config.ts
- packages/core/src/shared/color/channel.ts
- packages/core/src/ColorField/ColorFieldRoot.vue
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.
🔗 Linked issue
❓ Type of change
📚 Description
📸 Screenshots (if appropriate)
📝 Checklist
Summary by CodeRabbit
New Features
Documentation
Tests
Chores