feat: 新增 InputNumber 组件与优化 Slider 组件 #12
Conversation
WalkthroughAdds InputNumber and Slider components (types, styles, composables, TSX components, plugin entries, tests, and docs); centralizes Popover visibility/init/cleanup and updates Popover tests (disable teleport); wraps Timeline item icons with Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CInputNumber
participant Parent
Note right of CInputNumber `#DDFFDD`: normalize → validate → format
User->>CInputNumber: type / click controls / press arrows
CInputNumber->>CInputNumber: validate, clamp (min/max), apply precision
CInputNumber->>Parent: emit update:modelValue / input
User->>CInputNumber: blur
CInputNumber->>CInputNumber: format on blur
CInputNumber->>Parent: emit change / blur
sequenceDiagram
participant User
participant CSlider
participant Composables
participant Parent
Note right of Composables `#F0F8FF`: percent/value calc, interaction, tooltip
User->>CSlider: drag / click / keyboard
CSlider->>Composables: useSliderInteraction / useSliderKeyboard / useSliderCalculation
Composables->>Composables: compute percent ↔ value, clamp to [min,max]
Composables->>Parent: emit update:modelValue / input / change
CSlider->>Composables: useSliderStyle / useSliderTooltip
Composables->>CSlider: styles, tooltip content
sequenceDiagram
participant App
participant Popover
participant AutoUpdate
participant Document
Note right of Popover `#FFF4E6`: visibility normalization & lifecycle
App->>Popover: mount (props.visible true/false)
Popover->>Popover: actualVisible = Boolean(props.visible)
Popover->>AutoUpdate: init only when actualVisible true (nextTick)
Popover->>Document: attach listeners on visible
Document->>Popover: events (mousedown/keydown) handled
Popover->>AutoUpdate: cleanup and remove document listeners on hide/unmount
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (12)
packages/ccui/ui/slider/src/slider.scss (1)
157-319: Consider improving touch and keyboard affordancesThe base wrapper/button styling is solid, with hover/active/focus states and a responsive tweak. For even better UX on touch/keyboard:
- You might add
touch-action: none;to&__wrapperto further reduce unintended page scrolling during drags on some mobile browsers.- If you want to avoid always showing the focus ring on mouse click, consider switching
&:focusto&:focus-visiblefor the button.Both are optional and can be deferred.
packages/ccui/ui/slider/src/slider-types.ts (1)
3-112: Slider prop surface is well-structuredThe prop definitions cover the expected areas (range, size, input, tooltip, marks, a11y) and align with the SCSS modifiers (
large/default/small,vertical, placements, etc.). Nothing blocking here.If you want a tiny cleanup, you could introduce a shared formatter type, e.g.
type SliderFormatter = (value: number) => string, and reuse it forformatTooltip,formatValueText, andtipsRendererto reduce duplication, but this is purely ergonomic.packages/ccui/ui/slider/src/composables/use-slider.ts (2)
112-225: Global drag listeners are fine, but consider unmount cleanup / touch optionsThe drag logic (document-level
mousemove/mouseup/touchmove/touchendlisteners) is correct and cleaned up onhandleDragEnd. Two optional robustness tweaks:
- If the slider component can be unmounted while dragging (route change, v-if, etc.), it’s safer for the parent component to call
handleDragEndor otherwise remove listeners in anonBeforeUnmounthook to avoid lingering handlers.- To make
event.preventDefault()effective ontouchmoveacross more browsers, you may want to register thetouchmovelistener with{ passive: false }or rely on CSS liketouch-action: noneon the track/handle.Both are defensive improvements and not blockers.
341-365:useSliderInputassumes a single value; ensure it’s not used for range sliders
handleInputChangeclamps and then writes a scalar intocurrentValue:const clampedValue = Math.max(props.min, Math.min(props.max, value)) currentValue.value = clampedValue emit('change', clampedValue)If this composable is ever used when
props.range === true, it will collapse the range array into a single number and break the range semantics.If the design is “input box only for single-value sliders”, that’s fine—just ensure
useSliderInputis only wired in that mode (and perhaps guard with a runtime check + dev warning). Otherwise, you may want to extend this logic to edit the active handle in a range[start, end]instead of replacing the entire value.packages/ccui/ui/slider/src/composables/use-slider-tooltip.ts (1)
73-78: Consider simplifying boolean expression.The condition on line 74-77 can be refactored for better readability:
const shouldShowDefaultTooltipForButton = (index: number) => { - return props.showTooltip - && props.tipsRenderer === null - && props.showDefaultTooltip - && (hoverIndex.value === index || isDragging.value || props.persistent) + const isVisible = hoverIndex.value === index || isDragging.value || props.persistent + return props.showTooltip && props.tipsRenderer === null && props.showDefaultTooltip && isVisible }packages/ccui/ui/input-number/src/input-number.scss (1)
59-91: Consider adding validation for size-variant parameters.The
size-variantmixin accepts 7 parameters but lacks validation. While SCSS doesn't support runtime validation, consider adding comments documenting expected value types and units.Add parameter documentation:
+// @param {length} $width - Component width (e.g., 150px) +// @param {length} $height - Component height (e.g., 32px) +// @param {length} $font-size - Font size (e.g., 14px) +// @param {length} $inner-height - Inner input height (e.g., 30px) +// @param {length} $control-width - Control button width (e.g., 32px) +// @param {length} $svg-size - SVG icon size (e.g., 12px) +// @param {length} $padding - Inner padding (default: 0 11px) @mixin size-variant($width, $height, $font-size, $inner-height, $control-width, $svg-size, $padding: 0 11px) {packages/ccui/ui/input-number/src/input-number-types.ts (1)
34-91: Types and props look consistent; consider reusingInputNumberValuefor modelValueThe props and associated types are well aligned with the component API. For better cohesion and easier future changes, you could reuse the
InputNumberValuealias directly in the prop typing:modelValue: { type: Number as PropType<InputNumberValue>, default: undefined, },Purely a style/maintainability tweak; current code is correct.
packages/ccui/ui/slider/test/slider.test.ts (3)
164-177: ClarifytipsRenderer nullbehavior and assertionThe test name/comment say “should hide tooltip”, but the expectations are:
CTooltipexistscontentis''showTooltipis not explicitly set (relies on default)If the intended contract is “no tooltip at all when
tipsRendereris null”, this should assertexists() === false. If the intent is “tooltip shell exists but with empty content”, consider updating the test name/comment to reflect that to avoid confusion for future maintainers.
310-329: Strengthen Home/End keyboard tests with value assertionsThe Home/End keyboard test only checks that
update:modelValuewas emitted. To better guard against regressions, you might also assert the emitted values are clamped tominandmax, e.g. last emitted value equals 0 for Home and 100 for End.This would tie the test directly to the intended accessibility behavior of those keys.
649-661: Fix test name vs props forpersistent+showTooltipThe test is named
props - persistent false with showTooltip false, but the props passed are:persistent: true, showTooltip: false,The expectations (
CTooltipdoes not exist) match “showTooltip=false wins regardless of persistent”. To avoid confusion, consider either:
- Renaming the test to match the props (e.g. “persistent true with showTooltip false”), or
- Setting
persistent: falseif the name is what you want to assert.Behavior is fine; this is just for clarity.
packages/ccui/ui/input-number/src/input-number.tsx (2)
50-71: Normalize all entry points throughformatValuefor consistent behavior
formatValuecorrectly appliesallowEmpty, precision, and min/max, but:
setValueusesupdateValuedirectly.- The
watchonprops.modelValuealso assignsinnerValuedirectly.This means external updates (via instance API or v-model changes from the parent) can bypass normalization and display out-of-range or unformatted values, while user interactions go through
formatValue.For consistency, consider:
const instance: InputNumberInstance = { getValue: () => innerValue.value, setValue: (value: InputNumberValue) => updateValue(formatValue(value)), // … } watch( () => props.modelValue, (newValue) => { const formatted = formatValue(newValue as any) if (formatted !== innerValue.value) { innerValue.value = formatted } }, { immediate: true }, )This keeps the value invariant (min/max/precision/empty handling) regardless of how it’s set.
Also applies to: 191-212
248-261: Avoid writing ±Infinity asmin/maxHTML attributesWith the current defaults (
min = -Infinity,max = Infinity), the rendered input will get:<input min="-Infinity" max="Infinity" ...>Browsers tend to treat these as invalid and effectively ignore them, but it’s cleaner and less surprising to omit the attributes when unbounded. For example:
<input // ... min={Number.isFinite(props.min) ? props.min : undefined} max={Number.isFinite(props.max) ? props.max : undefined} />This preserves your internal clamping logic while keeping the DOM attributes semantically correct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
.github/workflows/buildComponents.yml(1 hunks).github/workflows/test.yml(1 hunks).ls-lint.yml(1 hunks)packages/ccui/ui/input-number/index.ts(1 hunks)packages/ccui/ui/input-number/src/input-number-types.ts(1 hunks)packages/ccui/ui/input-number/src/input-number.scss(1 hunks)packages/ccui/ui/input-number/src/input-number.tsx(1 hunks)packages/ccui/ui/input-number/test/input-number.test.ts(1 hunks)packages/ccui/ui/slider/index.ts(1 hunks)packages/ccui/ui/slider/src/composables/use-slider-tooltip.ts(1 hunks)packages/ccui/ui/slider/src/composables/use-slider.ts(1 hunks)packages/ccui/ui/slider/src/slider-types.ts(1 hunks)packages/ccui/ui/slider/src/slider.scss(1 hunks)packages/ccui/ui/slider/src/slider.tsx(1 hunks)packages/ccui/ui/slider/test/slider.test.ts(1 hunks)packages/docs/components/input-number/index.md(1 hunks)packages/docs/components/slider/index.md(1 hunks)packages/docs/components/timeline/index.md(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
packages/ccui/ui/slider/src/slider.tsx (4)
packages/ccui/ui/slider/src/slider-types.ts (2)
sliderProps(7-110)SliderProps(112-112)packages/ccui/ui/shared/hooks/use-namespace.ts (1)
useNamespace(31-47)packages/ccui/ui/slider/src/composables/use-slider.ts (7)
useSliderValue(5-22)useSliderCalculation(24-40)useSliderStyle(42-110)useSliderInteraction(112-225)useSliderKeyboard(227-286)useSliderMarks(288-339)useSliderInput(341-366)packages/ccui/ui/slider/src/composables/use-slider-tooltip.ts (1)
useSliderTooltip(5-126)
packages/ccui/ui/slider/src/composables/use-slider-tooltip.ts (1)
packages/ccui/ui/slider/src/slider-types.ts (1)
SliderProps(112-112)
packages/ccui/ui/input-number/src/input-number.tsx (2)
packages/ccui/ui/input-number/src/input-number-types.ts (4)
inputNumberProps(34-91)InputNumberProps(93-93)InputNumberValue(8-8)InputNumberInstance(114-127)packages/ccui/ui/shared/hooks/use-namespace.ts (1)
useNamespace(31-47)
packages/ccui/ui/slider/src/composables/use-slider.ts (1)
packages/ccui/ui/slider/src/slider-types.ts (1)
SliderProps(112-112)
packages/ccui/ui/slider/test/slider.test.ts (2)
packages/ccui/ui/shared/hooks/use-namespace.ts (1)
useNamespace(31-47)packages/ccui/ui/slider/index.ts (1)
Slider(8-8)
🪛 LanguageTool
packages/docs/components/slider/index.md
[uncategorized] ~13-~13: 动词的修饰一般为‘形容词（副词）+地+动词’。您的意思是否是：基本"地"滑
Context: ...要调整设置值时，如音量、亮度等 - 当需要选择范围值时 ## 基本用法 基本的滑块用法。 :::demo ```vue <script> import {...
(wb4)
packages/docs/components/input-number/index.md
[locale-violation] ~50-~50: 'input' é un xenismo. É preferíbel dicir "entrada"
Context: ... ::: ## 禁用状态 通过disabled` 属性指定是否禁用 input 组件。 :::demo ```vue <script> import { ...
(GL_BARBARISM_REPLACE)
[locale-violation] ~288-~288: 'show' é un xenismo. É preferíbel dicir "divertimento"
Context: ... | false | 是否允许空值 | | show-glow-style | boolean |...
(GL_BARBARISM_REPLACE)
[locale-violation] ~296-~296: 'Input' é un xenismo. É preferíbel dicir "entrada"
Context: ... | 在 Input 失去焦点时触发 | | focus | (event: Event) ...
(GL_BARBARISM_REPLACE)
[locale-violation] ~297-~297: 'Input' é un xenismo. É preferíbel dicir "entrada"
Context: ... | 在 Input 获得焦点时触发 | | input | `(currentValue: nu...
(GL_BARBARISM_REPLACE)
[locale-violation] ~298-~298: 'input' é un xenismo. É preferíbel dicir "entrada"
Context: ... | 在 Input 获得焦点时触发 | | input | `(currentValue: number | undefined)...
(GL_BARBARISM_REPLACE)
[locale-violation] ~298-~298: 'Input' é un xenismo. É preferíbel dicir "entrada"
Context: ...efined)` | 在 Input 值改变时触发 | ## InputNumber方法 | 方法名 | 说...
(GL_BARBARISM_REPLACE)
[locale-violation] ~304-~304: 'input' é un xenismo. É preferíbel dicir "entrada"
Context: ...----------------- | ---- | | focus | 使 input 获取焦点 | - | | blur | 使 input 失去焦点...
(GL_BARBARISM_REPLACE)
[locale-violation] ~305-~305: 'input' é un xenismo. É preferíbel dicir "entrada"
Context: ... | 使 input 获取焦点 | - | | blur | 使 input 失去焦点 | - | | select | 选中 input 中的文...
(GL_BARBARISM_REPLACE)
[locale-violation] ~306-~306: 'input' é un xenismo. É preferíbel dicir "entrada"
Context: ...| 使 input 失去焦点 | - | | select | 选中 input 中的文字 | - |
(GL_BARBARISM_REPLACE)
🔇 Additional comments (21)
.github/workflows/test.yml (1)
33-34: Test workflow now runs onlyccuitests
pnpm --filter ccui testis fine assumingccuiis the intended workspace package and you don’t need tests from other packages in this workflow. If the monorepo grows, consider whether this job should cover additional packages or stay scoped toccui..ls-lint.yml (1)
16-24: Globalnode_modulesignore is appropriateAdding a top-level
node_modulesignore is a good hardening of the lint config and complements the more specific package-level ignores..github/workflows/buildComponents.yml (1)
33-34: Build workflow scoped to CLI package
pnpm --filter cli build:componentslooks correct as long as theclipackage owns the components build entrypoint and there’s no need to run a broader workspace build here.packages/docs/components/timeline/index.md (1)
224-256: Timeline API tables formatting looks consistentThe revised tables for Timeline Slots, Timeline-Item Props, Events, and Slots have consistent column counts and clearer headers, which should render more predictably in the docs. No functional issues spotted.
packages/ccui/ui/slider/src/slider.scss (1)
134-155: With-input layout is clear and flexibleThe
&--with-inputblock cleanly handles the slider + input-number layout (row-reverse, flex, min-width safeguards). This should integrate well with the newCInputNumbercomponent.packages/docs/components/input-number/index.md (1)
1-306: LGTM! Well-structured documentation.The InputNumber documentation is comprehensive and well-organized, covering:
- Usage scenarios and basic examples
- All component states (disabled, readonly, etc.)
- Configuration options (min/max, step, precision, sizes, controls)
- Complete API reference with props, events, and methods
packages/docs/components/slider/index.md (1)
1-507: LGTM! Comprehensive Slider documentation.The Slider documentation thoroughly covers all component features:
- Multiple usage scenarios (basic, range, vertical, steps, marks, etc.)
- Tooltip customization and placement options
- Size variants and disabled states
- Accessibility features
- Complete API reference
packages/ccui/ui/input-number/test/input-number.test.ts (1)
1-291: LGTM! Comprehensive test coverage.The test suite thoroughly covers:
- Component rendering and structure
- v-model binding and reactivity
- User interactions (buttons, keyboard, input changes)
- Boundary validation (min/max limits)
- State variations (disabled, readonly, sizes, controls)
- Focus/blur events
- Edge cases (allowEmpty, precision)
- Public API methods exposure
The bilingual test descriptions (English and Chinese) are a nice touch for international teams.
packages/ccui/ui/input-number/index.ts (2)
10-17: Minor duplication: install method defined twice.Both the component itself (lines 4-6) and the default export object (lines 14-16) have identical
installmethods. While this provides flexibility (use as named import or default), it creates maintenance overhead.This pattern is acceptable if it's a project-wide convention for component exports.
4-6: Component name is properly defined; no issues found.The InputNumber component explicitly defines
name: 'CInputNumber'in itsdefineComponentconfiguration (line 12 ofinput-number.tsx), which ensures the non-null assertion in theinstallmethod is safe and justified.packages/ccui/ui/slider/index.ts (2)
10-17: LGTM! Consistent plugin pattern.The plugin wrapper follows the same pattern as InputNumber, providing both named and default exports with install methods. The explicit
voidreturn type annotation is a nice touch for type safety.
4-6: Component name is properly defined—non-null assertion is safe.The Slider component explicitly defines
name: 'CSlider'(line 20 inpackages/ccui/ui/slider/src/slider.tsx), confirming the non-null assertion inSlider.name!is appropriate and will not cause runtime errors.packages/ccui/ui/slider/src/composables/use-slider-tooltip.ts (2)
17-28: Clean tooltip formatting logic.The cascading logic correctly handles:
- Explicit null tipsRenderer (no custom tooltip)
- Custom formatTooltip function
- Custom tipsRenderer function
- Default string conversion fallback
101-108: LGTM! Smart placement logic.The tooltip placement logic correctly:
- Respects user-specified placement when provided
- Falls back to orientation-aware defaults (right for vertical, top for horizontal)
packages/ccui/ui/input-number/src/input-number.scss (3)
4-57: LGTM! Well-organized mixins.The mixins are well-structured and reusable:
disabled-state: Consistent disabled styling with hover state overridefocus-state: Accessible focus indicator with 2px shadowhide-controls: Clean button hiding utilitycontrol-button-base: DRY button styling with all interaction states
163-171: LGTM! Proper number input styling.Lines 163-171 correctly remove browser default number input spinners and apply textfield appearance for cross-browser consistency.
247-253: LGTM! Size variants properly implemented.Both size variants (lg and sm) correctly utilize the size-variant mixin with appropriate parameters. The values appear well-proportioned.
packages/ccui/ui/slider/src/slider.tsx (4)
68-106: LGTM! Well-structured button renderer.The
renderButtonhelper cleanly encapsulates:
- ARIA attributes for accessibility
- Event handlers for mouse, touch, and keyboard
- Conditional tooltip wrapping based on
showTooltipprop- Proper disabled state handling
159-175: LGTM! Proper InputNumber integration.The conditional rendering correctly:
- Only shows InputNumber when
showInputis true and not in range mode- Passes through all relevant props (modelValue, min, max, step, disabled, size, controls, precision)
- Wires up change handlers properly
194-210: LGTM! Efficient stops rendering.The stops rendering correctly:
- Calculates stop positions based on min, max, and step
- Uses proper positioning (bottom for vertical, left for horizontal)
- Generates appropriate number of stops
213-231: LGTM! Clean marks implementation.The marks rendering:
- Iterates over marks object keys
- Applies custom styles via
getMarkStyle- Renders both mark lines and labels
- Handles both string and object mark definitions
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/ccui/ui/timeline/src/timeline-item.tsx (1)
38-39: Functional implementation with minor pattern consideration.The icon rendering logic works correctly. Using
markRawhere prevents Vue from making the component reactive, which can be beneficial for performance.However, the typical Vue pattern is to apply
markRawat the component definition site rather than at render time. Ifprops.iconis already reactive when it reaches this point,markRawwon't undo that. Conversely, if it's not reactive,h()doesn't automatically make components reactive, somarkRawmay be unnecessary.Consider verifying whether:
- This addresses a specific reactivity issue you encountered
- Users should apply
markRawwhen passing icons (making this defensive)- Or if this can be simplified
Vue 3 markRaw best practices for component propspackages/ccui/ui/timeline/test/timeline.test.ts (1)
162-167: Test correctly validates icon component rendering.The test properly verifies component icon functionality by wrapping
IconComponentwithmarkRaw, which aligns with the implementation changes.Note that both the test (here) and the implementation (line 39 of timeline-item.tsx) apply
markRaw. If the implementation already handles this internally, tests and users shouldn't need to wrap their icons. This double-wrapping is safe but potentially redundant.Consider documenting whether:
- Users should wrap icon components with
markRawwhen passing them as props- Or if the component handles this internally (making user-side wrapping unnecessary)
This clarifies the API contract and avoids confusion.
packages/ccui/ui/popover/test/popover.test.ts (1)
527-555: Make the trigger explicit in the exposes-methods testIn the second mount (
wrapper = mount(Popover, { props: { content: 'Test', teleported: false } ... })), the test relies on clicking the trigger to show the popover before callingwrapper.vm.hide(). To avoid coupling this test to the component’s defaulttriggervalue, it would be clearer and more robust to specify the trigger explicitly:- wrapper = mount(Popover, { - props: { content: 'Test', teleported: false }, - slots: { default: '<button>Trigger</button>' }, - }) + wrapper = mount(Popover, { + props: { content: 'Test', trigger: 'click', teleported: false }, + slots: { default: '<button>Trigger</button>' }, + })This makes the intent obvious and keeps the test stable if the default trigger ever changes.
packages/ccui/ui/popover/src/popover.tsx (1)
274-285: Virtual trigger cleanup block is now orphanedIn
onUnmountedyou remove a number of event listeners fromprops.virtualRef:if (props.virtualTriggering && props.virtualRef) { const virtualEl = props.virtualRef virtualEl.removeEventListener('mouseenter', handleMouseEnter) virtualEl.removeEventListener('mouseleave', handleMouseLeave) virtualEl.removeEventListener('click', handleClick) virtualEl.removeEventListener('focus', handleFocus) virtualEl.removeEventListener('blur', handleBlur) virtualEl.removeEventListener('keydown', handleKeydown) virtualEl.removeEventListener('contextmenu', handleContextMenu) }But there is no corresponding
addEventListenerforvirtualRefanywhere in the current code path (listeners are now only attached viatriggerEventswhen!props.virtualTriggering). This makes the cleanup block effectively dead and potentially confusing for future maintenance.If virtual triggering is now intended to be manual-only (as the tests suggest), you can safely remove this cleanup block. If you still want interactive virtual triggers (hover/click/focus on an external element), the matching
addEventListenerlogic should be reintroduced instead.Given current usage, I recommend removing the dead cleanup to reflect the actual behavior:
- // 清理虚拟触发元素的事件监听 - if (props.virtualTriggering && props.virtualRef) { - const virtualEl = props.virtualRef - virtualEl.removeEventListener('mouseenter', handleMouseEnter) - virtualEl.removeEventListener('mouseleave', handleMouseLeave) - virtualEl.removeEventListener('click', handleClick) - virtualEl.removeEventListener('focus', handleFocus) - virtualEl.removeEventListener('blur', handleBlur) - virtualEl.removeEventListener('keydown', handleKeydown) - virtualEl.removeEventListener('contextmenu', handleContextMenu) - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/buildComponents.yml(2 hunks).github/workflows/test.yml(1 hunks)packages/ccui/ui/popover/src/popover.tsx(3 hunks)packages/ccui/ui/popover/test/popover.test.ts(24 hunks)packages/ccui/ui/timeline/src/timeline-item.tsx(2 hunks)packages/ccui/ui/timeline/test/timeline.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/test.yml
🧰 Additional context used
🧬 Code graph analysis (1)
packages/ccui/ui/popover/test/popover.test.ts (1)
packages/ccui/ui/tooltip/test/tooltip.test.ts (10)
it(182-286)it(288-307)wrapper(6-487)wrapper(344-374)it(465-486)it(95-180)wrapper(201-221)wrapper(96-110)wrapper(466-485)wrapper(289-306)
🔇 Additional comments (10)
.github/workflows/buildComponents.yml (2)
1-1: Verify workflow rename aligns with CI/CD strategy.The workflow was renamed from "Test" to "BuildComponents", reflecting a narrower focus on building components rather than running tests. Since the PR adds new tests for InputNumber and Slider components, confirm that test execution has been properly configured in an alternative workflow (e.g.,
test.ymlmentioned in the AI summary context).
33-34: All verification concerns are satisfied; no issues found.The review's concerns have been verified:
- ✅ Tests are properly configured in
test.ymlwhich runspnpm --filter vue3-ccui testusing vitest- ✅ Both InputNumber and Slider components have dedicated test files:
packages/ccui/ui/input-number/test/input-number.test.tspackages/ccui/ui/slider/test/slider.test.ts- ✅ No test coverage gaps exist; both new components are included in the test suite (16 of 19 component directories contain tests)
The separation of test execution (test.yml) from the build step (buildComponents.yml) is an appropriate CI design pattern.
packages/ccui/ui/timeline/src/timeline-item.tsx (1)
2-2: LGTM - Import addition for markRaw.The import of
markRawfrom Vue is correctly added to support the icon rendering optimization.packages/ccui/ui/timeline/test/timeline.test.ts (1)
3-3: LGTM - Import addition for test alignment.The
markRawimport correctly supports the updated test for component icon handling.packages/ccui/ui/popover/test/popover.test.ts (5)
41-57: Usingteleported: falsein most unit tests is a good, stable patternDisabling teleport for the majority of tests keeps the popper DOM local to the wrapper, which makes queries straightforward and avoids relying on global document structure. You still have a dedicated
teleport 功能test to validate the teleported path, so coverage remains balanced between internal and teleported rendering modes.Also applies to: 76-90, 92-107, 110-141, 143-189
211-237: Interaction and delay tests align well with the current visibility logicThe hover/focus/disabled and showAfter/hideAfter tests, combined with
hideAfter: 0where needed and fake timers for delay cases, accurately exercisedoShow/doHidebehavior and timer handling. The expectations around visibility toggling and delayed show/hide match the implementation, and usingteleported: falsekeeps the assertions focused on behavior rather than Teleport.Also applies to: 240-287
320-333: ARIA test correctly follows dynamic popper IDsReading the
idfrom.ccui-popover__popperand assertingaria-describedbymatches that value is the right move now that each popover instance has a unique, generated popper ID. This also makes the test resilient to future ID format changes as long as the trigger and popper stay in sync.
336-381: Outside-click and auto-close scenarios are covered appropriatelyThe click-outside tests (with and without
hideOnClickOutside) and theautoClosetest together validate the document-level mousedown/keydown handling and the auto-close timer. UsingattachTo: document.bodywhere needed andvi.useFakeTimersin the auto-close test aligns well with the implementation’s reliance on global events and timers.Also applies to: 384-432
434-469: Keyboard-trigger test nicely exercises the new focus +triggerKeysbehaviorThis test covers the full cycle for focus trigger with custom
triggerKeys:
- Focus shows the popover.
Entertoggles it off then back on.- Space (
' ') is normalized and toggles it off then back on.blurcloses it.That matches the
normalizeTriggerKeyandhandleKeydownlogic and gives good coverage for keyboard accessibility.packages/ccui/ui/popover/src/popover.tsx (1)
350-402: ARIA wiring and booleanactualVisiblenormalization look solidThe combination of:
actualVisibleas a strict boolean, andaria-describedby={actualVisible.value ? popperId : undefined}on the trigger, withid={popperId}androle="dialog"on the poppergives you per-instance unique IDs, ensures the ARIA reference only exists when the popper is actually in the DOM, and matches the updated tests. This is a clean, accessible setup for multiple popovers on the same page.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/ccui/ui/popover/src/popover.tsx(3 hunks)packages/ccui/ui/slider/src/composables/use-slider.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/ccui/ui/popover/src/popover.tsx (1)
packages/ccui/ui/tooltip/src/tooltip.tsx (7)
actualVisible(180-184)isControlled(25-25)props(24-24)newVal(203-211)props(192-192)props(141-150)props(158-162)
packages/ccui/ui/slider/src/composables/use-slider.ts (1)
packages/ccui/ui/slider/src/slider-types.ts (1)
SliderProps(112-112)
🔇 Additional comments (9)
packages/ccui/ui/popover/src/popover.tsx (4)
27-30: Good: Strict boolean coercion improves type safety.Wrapping the result with
Boolean()ensuresactualVisibleis always a strict boolean rather than a truthy/falsy value, which prevents edge cases and improves consistency.
258-270: Excellent: This fixes the initial visibility issue from the previous review.The updated
onMountednow correctly attaches document listeners (mousedown, keydown) when the component mounts withprops.visible === true. This ensures outside-click and Escape-to-close work immediately, even when visibility never toggles. The previous critical issue is resolved.
289-301: LGTM: Controlled-mode synchronization logic is correct.The watcher properly synchronizes
props.visibleto internal state with boolean coercion and change detection. Theupdate()call at Line 296 is slightly redundant since theactualVisiblewatcher will subsequently set upautoUpdate(which also callsupdate), but this is harmless and may provide immediate positioning.
302-317: LGTM: Core visibility lifecycle handling is sound.The
actualVisiblewatcher correctly manages theautoUpdatecleanup and document listener lifecycle. The defensive checks for element readiness (Line 305) and safe cleanup invocation (Line 306) prevent edge-case errors. This works correctly in tandem withonMountedfor comprehensive visibility management.packages/ccui/ui/slider/src/composables/use-slider.ts (5)
5-22: LGTM!The two-way binding implementation correctly emits both
update:modelValueandinputevents, properly supporting Vue's v-model pattern.
24-56: Excellent fix for step snapping!The calculation now correctly snaps values relative to
minand guards against degenerate configurations (range ≤ 0, step ≤ 0), addressing the issue raised in the previous review.
58-126: LGTM!The style calculations correctly handle both single and range modes across vertical and horizontal orientations.
243-302: LGTM!Keyboard navigation correctly handles both single and range modes, with proper clamping to prevent handles from crossing or exceeding bounds.
304-355: LGTM!The marks calculation correctly filters out-of-range values and provides flexible styling and labeling for both string and object mark definitions.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/ccui/ui/slider/src/composables/use-slider.ts(1 hunks)packages/ccui/ui/slider/src/slider.tsx(1 hunks)packages/docs/components/slider/index.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/ccui/ui/slider/src/slider.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
packages/ccui/ui/slider/src/composables/use-slider.ts (1)
packages/ccui/ui/slider/src/slider-types.ts (1)
SliderProps(112-112)
🪛 LanguageTool
packages/docs/components/slider/index.md
[uncategorized] ~13-~13: 动词的修饰一般为‘形容词（副词）+地+动词’。您的意思是否是：基本"地"滑
Context: ...要调整设置值时，如音量、亮度等 - 当需要选择范围值时 ## 基本用法 基本的滑块用法。 :::demo ```vue <script> import {...
(wb4)
🔇 Additional comments (8)
packages/docs/components/slider/index.md (2)
13-13: Disregard grammar suggestion at line 13.Static analysis flagged "基本的滑块用法" as a potential grammar issue, but this is correct Chinese (adjective + 的 + noun). The suggestion is a false positive and should be ignored.
1-541: API documentation accurately reflects component implementation.Verification confirms all 28 documented props exist in the component with correct types, default values, and options. The two documented events (change, input) are properly implemented. Documentation uses correct kebab-case naming convention for Vue props.
packages/ccui/ui/slider/src/composables/use-slider.ts (6)
5-22: LGTM: Clean v-model implementation.The computed wrapper correctly implements the standard Vue v-model pattern with proper event emissions.
24-56: LGTM: Step snapping now correctly aligned tomin.The calculation logic properly guards degenerate configurations and snaps steps relative to
minas suggested in the previous review. This fixes the issue where non-zero, non-multipleminvalues would produce incorrect snap points.
58-126: LGTM: Style calculations handle all orientation and mode combinations correctly.The computed styles properly handle single/range modes and vertical/horizontal orientations with appropriate CSS properties.
251-310: LGTM: Keyboard navigation properly handles all modes.The keyboard handler correctly implements arrow key stepping and Home/End jumps, with proper constraints for both range and single-value modes.
312-363: LGTM: Mark handling is robust and flexible.The marks composable correctly filters marks to the valid range and supports both string and object mark definitions with custom styling.
128-249: Memory leak concern has been successfully addressed.Verification confirms the
cleanupfunction is properly called in the consumer component'sonBeforeUnmounthook (packages/ccui/ui/slider/src/slider.tsx, lines 60-62). The cleanup wiring is correct and complete.
PR: 新增 InputNumber 组件与优化 Slider 组件
概述
此PR包含两个主要功能的实现：
InputNumber组件Slider组件测试
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests
Chores