feat(carousel): add draggable carousel#4006
Hidden character warning
Conversation
WalkthroughThe pull request introduces drag-and-drop functionality to carousel and collapse components. It adds draggable props, mouse event handlers (mousedown, mousemove, mouseup, mouseleave) in the renderless layer, wires events to Vue components, includes new demo files and E2E tests, updates component documentation, and adds associated styling. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PC.Vue as Carousel PC Component
participant VueAPI as Vue Renderless API
participant CoreLogic as Renderless Drag Logic
participant State as Component State
User->>PC.Vue: mousedown on carousel
activate PC.Vue
PC.Vue->>VueAPI: mousedown event triggered
activate VueAPI
VueAPI->>CoreLogic: mousedown({ props, state, api })
activate CoreLogic
CoreLogic->>State: Set isDragging = true
CoreLogic->>State: Record pointer position
CoreLogic->>State: Clear/pause timer
deactivate CoreLogic
deactivate VueAPI
deactivate PC.Vue
User->>PC.Vue: mousemove while dragging
activate PC.Vue
PC.Vue->>VueAPI: mousemove event triggered
activate VueAPI
VueAPI->>CoreLogic: mousemove({ props, state, vm })
activate CoreLogic
CoreLogic->>State: Calculate delta from movement
CoreLogic->>State: Determine drag direction
CoreLogic->>State: Update carousel items offset
deactivate CoreLogic
deactivate VueAPI
deactivate PC.Vue
User->>PC.Vue: mouseup to complete drag
activate PC.Vue
PC.Vue->>VueAPI: mouseup event triggered
activate VueAPI
VueAPI->>CoreLogic: mouseup({ props, state, api })
activate CoreLogic
CoreLogic->>State: Set isDragging = false
CoreLogic->>State: Calculate slide change
CoreLogic->>State: Restart timer
CoreLogic->>State: Reset delta
deactivate CoreLogic
deactivate VueAPI
deactivate PC.Vue
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@examples/sites/demos/mobile-first/app/collapse/webdoc/collapse.js`:
- Around line 158-169: The demo description incorrectly refers to a "draggable
slot" but the feature is exposed via the draggable prop; update the descriptive
text in the demo entry with demoId 'draggable' (the desc values) to mention the
draggable prop instead of a slot (e.g., "by configuring the built-in draggable
prop of Collapse" for both 'zh-CN' and 'en-US') so the documentation matches the
implementation.
In `@examples/sites/demos/pc/app/carousel/draggable.spec.ts`:
- Around line 29-32: The test dispatches 'touchstart'/'touchend' via
carousel.dispatchEvent without supplying touch point data, but the component's
touch handler reads event.touches[0].clientX/Y; update the test to dispatch
proper TouchEvent objects that include a touches array with a Touch-like point
(clientX/clientY) so the handler can read coordinates. Replace the simple
dispatch calls for 'touchstart' and 'touchend' with creation of TouchEvent
instances (or a CustomEvent with a touches property) containing a touch point
matching the carousel element (refer to carousel.dispatchEvent and the
component's touchstart handler expecting event.touches[0].clientX/Y) and use
those events in the awaits.
In `@packages/renderless/src/carousel/index.ts`:
- Around line 399-404: The current code calls throttle(100, true, () => true)
inside a conditional, but throttle returns a throttled function (not a boolean),
so the early return is never reached; fix by creating the throttled wrapper once
(e.g., define a const throttledHandler = throttle(100, true, originalHandler)
during setup or module init) and then call throttledHandler() from the place
where the conditional currently exists (replace the if-block using throttle(...)
with a call to the pre-created throttled function); reference the throttle
function and the handler wrapper (throttledHandler/originalHandler) when
updating the code.
In `@packages/vue/src/carousel/src/pc.vue`:
- Around line 18-22: There are two `@mouseleave` handlers (handleMouseLeave and
mouseleave) which both fire on leave and can double-trigger timer logic; remove
the duplicate directive (remove `@mouseleave`="mouseleave") and consolidate
behavior by updating handleMouseLeave in the renderless layer to also perform
the drag cleanup currently in mouseleave (e.g., stop drag state, clear flags,
and call any drag-specific cleanup) so a single handler manages both timer
restart and drag cleanup consistently.
🧹 Nitpick comments (7)
packages/theme/src/carousel/index.less (1)
257-261: Align the draggable hook with the CSS prefix and drag-state cursor.Hard-coding
.tiny-carousel--draggablebypasses@{carousel-prefix-cls}overrides, andcursor: grabbingis usually reserved for the active drag state. Consider using the prefix variable and a grab/grabbing split.♻️ Proposed refactor
-.tiny-carousel--draggable { - //鼠标效果 - cursor: grabbing; - user-select: none; -} +.@{carousel-prefix-cls}--draggable { + // 鼠标效果 + cursor: grab; + user-select: none; + + &:active { + cursor: grabbing; + } +}examples/sites/demos/apis/statistic.js (1)
106-109: Represent the string default with quotes for clarity.Other string defaults are shown with quotes (e.g.,
'hover'). UsingdefaultValue: ','may render ambiguously; considerdefaultValue: "','".♻️ Proposed tweak
- defaultValue: ',', + defaultValue: "','",examples/sites/demos/pc/app/carousel/draggable.spec.ts (1)
23-27: AvoidwaitForTimeoutin favor of explicit conditions.Using
page.waitForTimeout()leads to flaky tests. Consider usingpage.waitForSelector()or explicit state assertions instead. Also,dispatchEventmay not properly initialize event coordinates - consider usingpage.mouse.down(),page.mouse.move(),page.mouse.up()for more realistic simulation.Suggested approach using Playwright's mouse API
- await carousel.dispatchEvent('mousedown') - await page.waitForTimeout(50) - await carousel.dispatchEvent('mousemove', { clientX: 100, clientY: 100 }) - await page.waitForTimeout(50) - await carousel.dispatchEvent('mouseup') + const box = await carousel.boundingBox() + if (box) { + const startX = box.x + box.width / 2 + const startY = box.y + box.height / 2 + await page.mouse.move(startX, startY) + await page.mouse.down() + await page.mouse.move(startX - 100, startY) + await page.mouse.up() + }packages/vue/src/carousel/src/pc.vue (1)
14-14: Class binding should reflect dragging state, not just the prop.The
tiny-carousel--draggableclass (which setscursor: grabbing) is bound to thedraggableprop. This means the grabbing cursor will always show when the prop is enabled. Typically,cursor: grabshould show when hoverable, andcursor: grabbingonly while actively dragging.Suggested: Bind class to active dragging state
- :class="{ 'tiny-carousel--card': type === 'card', 'tiny-carousel--draggable': draggable }" + :class="{ 'tiny-carousel--card': type === 'card', 'tiny-carousel--draggable': draggable && state.moving }"And consider adding a separate
tiny-carousel--drag-enabledclass for the defaultcursor: grabstyle.packages/renderless/src/carousel/vue.ts (1)
142-146: Minor formatting inconsistencies.Missing spaces after commas in the object destructuring patterns, and the trailing comment on line 146 seems misplaced.
Formatting fix
canActive: canActive(props), - mousedown: mousedown({ props, state, api }), - mousemove: mousemove({ props, state, vm }), - mouseup: mouseup({ props,state, api }), - mouseleave: mouseleave({ props,state, api }), - //拖动相关的事件 + mousedown: mousedown({ props, state, api }), + mousemove: mousemove({ props, state, vm }), + mouseup: mouseup({ props, state, api }), + mouseleave: mouseleave({ props, state, api })packages/renderless/src/carousel/index.ts (2)
391-436: Consider extracting shared logic with touch handlers.The
mousemoveandtouchmovehandlers share nearly identical logic (direction detection, delta calculation, boundary checks, item updates). Similarly,mouseupandtouchendare duplicated. This increases maintenance burden.Consider extracting the shared movement logic into a helper function that both mouse and touch handlers can use:
const handleDragMove = ({ state, props, carousel }, clientX, clientY) => { // shared delta calculation, direction detection, item updates }Then both handlers call it with the appropriate coordinates from their respective event objects.
464-474: Unusedeventparameter.The
eventparameter inmouseleaveis declared but never used. Consider removing it or prefixing with underscore to indicate intentional non-use.Remove unused parameter
export const mouseleave = ({ state, api }) => - (event) => { + () => { if (state.moving) {
| //draggable | ||
| { | ||
| demoId: 'draggable', | ||
| name: { | ||
| 'zh-CN': '可拖拽', | ||
| 'en-US': 'Draggable' | ||
| }, | ||
| desc: { | ||
| 'zh-CN': '<p>通过配置 Collapse 内置的 draggable 插槽实现拖拽<p>', | ||
| 'en-US': '<p>Drag and drop is implemented by configuring the built-in draggable slot of Collapse.</p>' | ||
| }, | ||
| codeFiles: ['draggable.vue'] |
There was a problem hiding this comment.
Doc text should reference the draggable prop, not a slot.
The demo uses a prop (draggable), so “draggable slot” is misleading.
✏️ Suggested wording
- desc: {
- 'zh-CN': '<p>通过配置 Collapse 内置的 draggable 插槽实现拖拽<p>',
- 'en-US': '<p>Drag and drop is implemented by configuring the built-in draggable slot of Collapse.</p>'
- },
+ desc: {
+ 'zh-CN': '<p>通过配置 Collapse 的 <code>draggable</code> 属性实现拖拽<p>',
+ 'en-US': '<p>Enable drag by setting the <code>draggable</code> prop on Collapse.</p>'
+ },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| //draggable | |
| { | |
| demoId: 'draggable', | |
| name: { | |
| 'zh-CN': '可拖拽', | |
| 'en-US': 'Draggable' | |
| }, | |
| desc: { | |
| 'zh-CN': '<p>通过配置 Collapse 内置的 draggable 插槽实现拖拽<p>', | |
| 'en-US': '<p>Drag and drop is implemented by configuring the built-in draggable slot of Collapse.</p>' | |
| }, | |
| codeFiles: ['draggable.vue'] | |
| //draggable | |
| { | |
| demoId: 'draggable', | |
| name: { | |
| 'zh-CN': '可拖拽', | |
| 'en-US': 'Draggable' | |
| }, | |
| desc: { | |
| 'zh-CN': '<p>通过配置 Collapse 的 <code>draggable</code> 属性实现拖拽<p>', | |
| 'en-US': '<p>Enable drag by setting the <code>draggable</code> prop on Collapse.</p>' | |
| }, | |
| codeFiles: ['draggable.vue'] |
🤖 Prompt for AI Agents
In `@examples/sites/demos/mobile-first/app/collapse/webdoc/collapse.js` around
lines 158 - 169, The demo description incorrectly refers to a "draggable slot"
but the feature is exposed via the draggable prop; update the descriptive text
in the demo entry with demoId 'draggable' (the desc values) to mention the
draggable prop instead of a slot (e.g., "by configuring the built-in draggable
prop of Collapse" for both 'zh-CN' and 'en-US') so the documentation matches the
implementation.
| // 6. 测试触摸事件可以触发 | ||
| await carousel.dispatchEvent('touchstart') | ||
| await page.waitForTimeout(50) | ||
| await carousel.dispatchEvent('touchend') |
There was a problem hiding this comment.
Touch events lack required touch point data.
The touchstart event handler in the renderless code expects event.touches[0].clientX/Y. Dispatching events without this data won't properly test touch functionality.
Consider using proper touch event initialization
- await carousel.dispatchEvent('touchstart')
- await page.waitForTimeout(50)
- await carousel.dispatchEvent('touchend')
+ await carousel.evaluate((el) => {
+ const rect = el.getBoundingClientRect()
+ const touch = new Touch({
+ identifier: 1,
+ target: el,
+ clientX: rect.left + rect.width / 2,
+ clientY: rect.top + rect.height / 2
+ })
+ el.dispatchEvent(new TouchEvent('touchstart', { touches: [touch] }))
+ el.dispatchEvent(new TouchEvent('touchend', { changedTouches: [touch] }))
+ })🤖 Prompt for AI Agents
In `@examples/sites/demos/pc/app/carousel/draggable.spec.ts` around lines 29 - 32,
The test dispatches 'touchstart'/'touchend' via carousel.dispatchEvent without
supplying touch point data, but the component's touch handler reads
event.touches[0].clientX/Y; update the test to dispatch proper TouchEvent
objects that include a touches array with a Touch-like point (clientX/clientY)
so the handler can read coordinates. Replace the simple dispatch calls for
'touchstart' and 'touchend' with creation of TouchEvent instances (or a
CustomEvent with a touches property) containing a touch point matching the
carousel element (refer to carousel.dispatchEvent and the component's touchstart
handler expecting event.touches[0].clientX/Y) and use those events in the
awaits.
| @mouseleave.stop="handleMouseLeave" | ||
| @mousedown="mousedown" | ||
| @mousemove="mousemove" | ||
| @mouseup="mouseup" | ||
| @mouseleave="mouseleave" |
There was a problem hiding this comment.
Duplicate @mouseleave event handlers may cause issues.
There are two @mouseleave handlers: handleMouseLeave (line 18) and mouseleave (line 22). Both will fire on mouseleave, potentially causing api.startTimer() to be called twice and creating race conditions with the timer.
Consider consolidating the logic into a single handler or ensuring the drag-related mouseleave handler properly coordinates with handleMouseLeave.
Option: Remove duplicate and handle both concerns in one handler
`@mouseenter.stop`="handleMouseEnter"
- `@mouseleave.stop`="handleMouseLeave"
+ `@mouseleave.stop`="handleMouseLeave"
`@mousedown`="mousedown"
`@mousemove`="mousemove"
`@mouseup`="mouseup"
- `@mouseleave`="mouseleave"Then update handleMouseLeave in the renderless layer to also call the drag cleanup logic from mouseleave.
🤖 Prompt for AI Agents
In `@packages/vue/src/carousel/src/pc.vue` around lines 18 - 22, There are two
`@mouseleave` handlers (handleMouseLeave and mouseleave) which both fire on leave
and can double-trigger timer logic; remove the duplicate directive (remove
`@mouseleave`="mouseleave") and consolidate behavior by updating handleMouseLeave
in the renderless layer to also perform the drag cleanup currently in mouseleave
(e.g., stop drag state, clear flags, and call any drag-specific cleanup) so a
single handler manages both timer restart and drag cleanup consistently.
| }, | ||
| //draggable | ||
| { | ||
| name: 'draggable', |
| mfDemo: '' | ||
| }, | ||
| { | ||
| name: 'use-animation', |
There was a problem hiding this comment.
其它组件有类似属性，好像 animate:true 这样来用。 所以不建议使用 use-animation这个属性名字
| }, | ||
| mode: ['pc', 'mobile-first'], | ||
| pcDemo: 'draggable', | ||
| mfDemo: 'draggable' |
| @@ -133,4 +169,4 @@ interface ITitleType { | |||
| ` | |||
| } | |||
| ] | |||
| } | |||
| } No newline at end of file | |||
There was a problem hiding this comment.
@ourcx 该 PR 用于给 Carousel 组件增加 draggable 特性，不应该修改 statistic 和 collapse 组件，如果要修改statistic 和 collapse 组件，建议另开新 PR
ourcx
force-pushed
the
Feature—Carousel
branch
from
8d2262a to
6798c24
Compare
|
把 static 的修改删除了，因为当时切换分支的时候的遗漏 |
The static modification was deleted because it was omitted when switching branches. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@examples/sites/demos/apis/carousel.js`:
- Around line 203-218: Update the API docs for the "draggable" prop so its
defaultValue matches the implementation: change defaultValue from 'false' to
'true' (to reflect the prop default: true in
packages/vue/src/carousel/src/index.ts), and remove the redundant comment
"//draggable" above the prop entry since the name field already documents it.
In `@packages/renderless/src/carousel/index.ts`:
- Around line 458-468: The mouseleave handler (mouseleve) currently only resets
state.moving and clears deltas but omits the same finalization logic used in
mouseup; update the mouseleave function to mirror mouseup by checking each
item's itemsTranslate delta and calling resetAnimatingMf() when delta > 1 for
the affected item(s), then call api.startTimer(); also remove the unused event
parameter from the mouseleave signature to avoid an unused variable. Ensure you
reference the same symbols: mouseleave, state.moving, state.itemsTranslate,
state.items, setDelta, resetAnimatingMf, and api.startTimer when making the
change.
🧹 Nitpick comments (3)
packages/theme/src/carousel/index.less (1)
257-261: Cursor state should differentiate between idle and active dragging.The
cursor: grabbingstyle is applied statically whendraggableis true, butgrabbingshould only show while actively dragging. The idle state should usecursor: grabto indicate the element is draggable.Consider splitting into two states:
- Default draggable state:
cursor: grab- Active dragging state:
cursor: grabbing♻️ Proposed fix
.tiny-carousel--draggable { - //鼠标效果 - cursor: grabbing; + cursor: grab; user-select: none; + + &.is-dragging { + cursor: grabbing; + } }Then toggle the
is-draggingclass dynamically in the component whenstate.movingis true.packages/renderless/src/carousel/index.ts (1)
399-399: Remove stale comment.The comment "增加节流 - 我觉得可能引发一些问题，后续可以多看这个逻辑" (adding throttle - I think this might cause some problems, need to look at this logic later) is now stale since the throttle is correctly applied by wrapping the entire handler. This leftover comment may confuse future maintainers.
🧹 Remove stale comment
- //增加节流 - 我觉得可能引发一些问题，后续可以多看这个逻辑 - state.deltaPos.X = event.clientX - state.startPos.Xpackages/vue/src/carousel/src/pc.vue (1)
14-14: Consider dynamic class for active drag state.The
tiny-carousel--draggableclass is applied statically based on thedraggableprop. To properly showcursor: grabbingonly while dragging (as suggested in the CSS review), you'll need to also bind a class based onstate.moving:♻️ Proposed enhancement
- :class="{ 'tiny-carousel--card': type === 'card', 'tiny-carousel--draggable': draggable }" + :class="{ + 'tiny-carousel--card': type === 'card', + 'tiny-carousel--draggable': draggable, + 'is-dragging': draggable && state.moving + }"
| //draggable | ||
| { | ||
| name: 'draggable', | ||
| type: 'boolean', | ||
| defaultValue: 'false', | ||
| desc: { | ||
| 'zh-CN': '是否允许拖拽切换幻灯片', | ||
| 'en-US': 'Whether to allow dragging to switch slides' | ||
| }, | ||
| mode: ['pc', 'mobile-first'], | ||
| pcDemo: 'draggable', | ||
| mfDemo: 'draggable', | ||
| meta: { | ||
| stable: '3.29.0' | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the default value in the component definition
rg -n "draggable" packages/vue/src/carousel/src/index.ts -A 3 -B 1Repository: opentiny/tiny-vue
Length of output: 167
🏁 Script executed:
#!/bin/bash
# Check the actual code in the API docs file at the specified lines
sed -n '203,218p' examples/sites/demos/apis/carousel.jsRepository: opentiny/tiny-vue
Length of output: 483
Fix default value mismatch and remove redundant comment.
The API documentation specifies defaultValue: 'false', but the implementation in packages/vue/src/carousel/src/index.ts defines default: true. Correct the API docs to match the actual default value.
Also remove the redundant comment //draggable on line 203; the name field below is self-explanatory.
🤖 Prompt for AI Agents
In `@examples/sites/demos/apis/carousel.js` around lines 203 - 218, Update the API
docs for the "draggable" prop so its defaultValue matches the implementation:
change defaultValue from 'false' to 'true' (to reflect the prop default: true in
packages/vue/src/carousel/src/index.ts), and remove the redundant comment
"//draggable" above the prop entry since the name field already documents it.
| // 鼠标离开窗口 | ||
| export const mouseleave = | ||
| ({ state, api }) => | ||
| (event) => { | ||
| if (state.moving) { | ||
| state.moving = false | ||
| state.itemsTranslate.forEach((item, index) => { | ||
| state.items[index].setDelta(0) | ||
| }) | ||
| api.startTimer() | ||
| } |
There was a problem hiding this comment.
mouseleave cleanup is incomplete compared to mouseup.
When the mouse leaves while dragging, mouseleave only resets moving and clears deltas, but unlike mouseup (lines 447-453), it doesn't call resetAnimatingMf() when delta > 1. This inconsistency could leave carousel items in an intermediate visual state.
Also, the event parameter is unused.
🛠️ Suggested fix for consistent cleanup
export const mouseleave =
({ state, api }) =>
- (event) => {
+ () => {
if (state.moving) {
state.moving = false
state.itemsTranslate.forEach((item, index) => {
state.items[index].setDelta(0)
})
+ if (Math.abs(state.delta) > 1) {
+ state.items.forEach((item) => {
+ item.resetAnimatingMf()
+ })
+ }
api.startTimer()
}
}🤖 Prompt for AI Agents
In `@packages/renderless/src/carousel/index.ts` around lines 458 - 468, The
mouseleave handler (mouseleve) currently only resets state.moving and clears
deltas but omits the same finalization logic used in mouseup; update the
mouseleave function to mirror mouseup by checking each item's itemsTranslate
delta and calling resetAnimatingMf() when delta > 1 for the affected item(s),
then call api.startTimer(); also remove the unused event parameter from the
mouseleave signature to avoid an unused variable. Ensure you reference the same
symbols: mouseleave, state.moving, state.itemsTranslate, state.items, setDelta,
resetAnimatingMf, and api.startTimer when making the change.
5 participants
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: 3873
What is the new behavior?
Does this PR introduce a breaking change?
Other information
增加了拖动可以切换走马灯，还有增加了文档的例子和测试
Summary by CodeRabbit
Release Notes
New Features
draggableprop to Carousel component, allowing users to drag to switch slides.Documentation
Tests