feat(modal): supports setting the headerDragable property #3918
Conversation
WalkthroughA new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
examples/sites/demos/pc/app/modal/webdoc/modal.js (1)
79-87: Documentation inconsistency: default value conflict.The Chinese documentation states the default value for
header-dragableistrue(line 81), which matches the component definition but conflicts with the API documentation inexamples/sites/demos/apis/modal.jsthat specifiesdefaultValue: 'false'.Additionally, the English translation (lines 84-87) is incomplete and doesn't mention the
header-dragableproperty at all.Action required:
- Align the default value documentation with the actual component behavior
- Add complete English translation for the
header-dragableproperty🔎 Update English documentation:
'en-US': ` Use the <code>show-header</code> property to set whether to display headers. The default value is <code>true</code><br> + Use the <code>header-dragable</code> property to set whether the header can be dragged. The default value is <code>true</code><br> Set the title of the window through the <code>title</code> property. <br> `
🧹 Nitpick comments (1)
packages/vue/src/modal/src/index.ts (1)
99-99: Fix misspelling: "Dragable" should be "Draggable"This is not an intentional naming convention. Verification shows the entire codebase uses the correct "Draggable" spelling: tree, dialog-box, popeditor, toggle-menu, tabs, tree-menu, drawer, and 15+ other components all use "draggable". The modal component is the only outlier with the misspelled "dragable" variant.
Rename
headerDragabletoheaderDraggableandfooterDragabletofooterDraggableacross:
- Component props in
packages/vue/src/modal/src/index.ts- Template usage in
packages/vue/src/modal/src/pc.vue- Demo files and API documentation
- Type definitions
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
examples/sites/demos/apis/modal.js(1 hunks)examples/sites/demos/pc/app/modal/modal-header.vue(2 hunks)examples/sites/demos/pc/app/modal/webdoc/modal.js(2 hunks)packages/vue/src/modal/src/index.ts(1 hunks)packages/vue/src/modal/src/pc.vue(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR E2E Test (pnpm test:e2e3)
🔇 Additional comments (4)
packages/vue/src/modal/src/pc.vue (2)
69-69: LGTM: Prop correctly added to component.The
headerDragableprop is properly included in the props array.
178-178: LGTM: Conditional drag behavior implemented correctly.The mousedown handler is correctly bound conditionally based on the
headerDragableprop value. WhenheaderDragableistrue, the header can be dragged viathis.mousedownEvent; whenfalse, an empty function disables dragging.This implementation is consistent with the
footerDragablepattern at line 289.examples/sites/demos/pc/app/modal/webdoc/modal.js (1)
98-111: LGTM: footer-dragable documentation updated consistently.The documentation correctly describes the
footer-dragableproperty and its default value offalsein both Chinese and English, with proper formatting and clear explanations.examples/sites/demos/apis/modal.js (1)
116-125: Review the default value type forfooter-dragablein the API documentation.The
footer-dragableproperty hasdefaultValue: 'false'as a string instead of the booleanfalse. The component prop definition inindex.tsdeclares it asfooterDragable: Boolean, so the API documentation should use a boolean value for consistency. Additionally, existing demos that use this feature (e.g.,modal-footer.vue) explicitly setfooterDragable: true, which suggests the documented default offalseis intentional and already aligned with current behavior.
| { | ||
| name: 'header-dragable', | ||
| type: 'boolean', | ||
| defaultValue: 'false', | ||
| desc: { | ||
| 'zh-CN': '控制标题可拖拽', | ||
| 'en-US': 'Control header dragable' | ||
| }, | ||
| mode: ['pc'], | ||
| pcDemo: 'modal-header' | ||
| }, |
There was a problem hiding this comment.
Critical: API documentation default value conflicts with component definition.
The API documentation specifies defaultValue: 'false' for header-dragable, but the component definition in packages/vue/src/modal/src/index.ts (line 99) has default: true. This inconsistency will confuse users about the actual default behavior.
Action required: Align the default value across component definition and API documentation. Based on the context:
- The demo explicitly sets
headerDragable: falseto disable dragging - This suggests the default should be
true(dragging enabled) - Update this API doc to reflect
defaultValue: 'true'
🔎 Apply this fix if default should be true:
{
name: 'header-dragable',
type: 'boolean',
- defaultValue: 'false',
+ defaultValue: 'true',
desc: {
'zh-CN': '控制标题可拖拽',
'en-US': 'Control header dragable'
},
mode: ['pc'],
pcDemo: 'modal-header'
},📝 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.
| { | |
| name: 'header-dragable', | |
| type: 'boolean', | |
| defaultValue: 'false', | |
| desc: { | |
| 'zh-CN': '控制标题可拖拽', | |
| 'en-US': 'Control header dragable' | |
| }, | |
| mode: ['pc'], | |
| pcDemo: 'modal-header' | |
| }, | |
| { | |
| name: 'header-dragable', | |
| type: 'boolean', | |
| defaultValue: 'true', | |
| desc: { | |
| 'zh-CN': '控制标题可拖拽', | |
| 'en-US': 'Control header dragable' | |
| }, | |
| mode: ['pc'], | |
| pcDemo: 'modal-header' | |
| }, |
🤖 Prompt for AI Agents
In examples/sites/demos/apis/modal.js around lines 126 to 136, the API doc sets
defaultValue: 'false' for header-dragable but the component's prop default in
packages/vue/src/modal/src/index.ts (line 99) is true; update the API
documentation to defaultValue: 'true' so it matches the component (or
alternatively change the component default to false if you intend docs to be
source of truth) — make the values consistent and ensure the string boolean is
'true' in the doc.
| <tiny-modal | ||
| v-model="show" | ||
| title="自定义弹窗标题" | ||
| message="窗口内容" | ||
| show-header | ||
| show-footer | ||
| :header-dragable="false" | ||
| > | ||
| </tiny-modal> |
There was a problem hiding this comment.
Demo disables the feature it's meant to showcase.
The demo sets :header-dragable="false", which disables header dragging. If the purpose of this demo is to showcase the new header-dragable feature, it would be more intuitive to demonstrate the feature enabled (set to true) so users can interact with draggable headers.
Recommendation: Consider one of these approaches:
- Set
header-dragable="true"to demonstrate the dragging capability - Show both enabled and disabled states with two separate modals
- Add explanatory text indicating this demo shows how to disable dragging
🔎 Option 1: Enable the feature in the demo:
<tiny-modal
v-model="show"
title="自定义弹窗标题"
message="窗口内容"
show-header
show-footer
- :header-dragable="false"
+ :header-dragable="true"
>
</tiny-modal>📝 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.
| <tiny-modal | |
| v-model="show" | |
| title="自定义弹窗标题" | |
| message="窗口内容" | |
| show-header | |
| show-footer | |
| :header-dragable="false" | |
| > | |
| </tiny-modal> | |
| <tiny-modal | |
| v-model="show" | |
| title="自定义弹窗标题" | |
| message="窗口内容" | |
| show-header | |
| show-footer | |
| :header-dragable="true" | |
| > | |
| </tiny-modal> |
🤖 Prompt for AI Agents
In examples/sites/demos/pc/app/modal/modal-header.vue around lines 10 to 18 the
demo unnecessarily disables the feature it intends to showcase by setting
:header-dragable="false"; change the demo to demonstrate the draggable header by
setting header-dragable to true (or add a second modal demonstrating both true
and false, or add text explaining this one shows the disabled state) so users
can see the drag behavior — update the prop to true or add the alternate
modal/text accordingly.
| Modal.alert({ | ||
| message: '窗口内容', | ||
| title: '自定义弹窗标题', | ||
| showHeader: true, | ||
| showFooter: true, | ||
| headerDragable: false | ||
| }) |
There was a problem hiding this comment.
Demo disables the feature in functional call as well.
Similar to the template usage, the functional call also sets headerDragable: false, which disables the dragging feature. For a feature introduction demo, it would be more effective to show the feature enabled.
🔎 Consider enabling the feature:
Modal.alert({
message: '窗口内容',
title: '自定义弹窗标题',
showHeader: true,
showFooter: true,
- headerDragable: false
+ headerDragable: true
})📝 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.
| Modal.alert({ | |
| message: '窗口内容', | |
| title: '自定义弹窗标题', | |
| showHeader: true, | |
| showFooter: true, | |
| headerDragable: false | |
| }) | |
| Modal.alert({ | |
| message: '窗口内容', | |
| title: '自定义弹窗标题', | |
| showHeader: true, | |
| showFooter: true, | |
| headerDragable: true | |
| }) |
🤖 Prompt for AI Agents
In examples/sites/demos/pc/app/modal/modal-header.vue around lines 38 to 44, the
functional demo call disables dragging by setting headerDragable: false; update
this to enable the feature (set headerDragable: true or remove the property if
true is the default) so the demo demonstrates draggable headers as intended.
| customClass: String, // mf 属性 | ||
| confirmBtnProps: { type: Object, default: () => ({}) }, | ||
| cancelBtnProps: { type: Object, default: () => ({}) }, | ||
| headerDragable: { type: Boolean, default: true }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the files mentioned in the review
echo "=== Looking for the component index.ts file ==="
fd "index.ts" packages/vue/src/modal/src 2>/dev/null || echo "Not found in expected path"
echo ""
echo "=== Looking for the modal API docs ==="
fd "modal.js" examples/sites/demos/apis 2>/dev/null || echo "Not found in expected path"
echo ""
echo "=== Looking for modal-header.vue demo ==="
fd "modal-header.vue" examples/sites/demos 2>/dev/null || echo "Not found in expected path"Repository: opentiny/tiny-vue
Length of output: 313
🏁 Script executed:
#!/bin/bash
echo "=== packages/vue/src/modal/src/index.ts (around line 99) ==="
sed -n '90,110p' packages/vue/src/modal/src/index.ts
echo ""
echo "=== examples/sites/demos/apis/modal.js (around line 129) ==="
sed -n '120,140p' examples/sites/demos/apis/modal.js
echo ""
echo "=== examples/sites/demos/pc/app/modal/modal-header.vue ==="
cat examples/sites/demos/pc/app/modal/modal-header.vueRepository: opentiny/tiny-vue
Length of output: 2404
🏁 Script executed:
#!/bin/bash
echo "=== Confirming headerDragable default values ==="
echo ""
echo "1. Component definition (index.ts):"
grep -n "headerDragable" packages/vue/src/modal/src/index.ts | head -1
echo ""
echo "2. API documentation (modal.js):"
grep -n "header-dragable" examples/sites/demos/apis/modal.js -A 3 | head -4
echo ""
echo "3. Summary:"
echo "Component default: true (Boolean)"
echo "API docs default: 'false' (string)"
echo "Inconsistency: CONFIRMED"Repository: opentiny/tiny-vue
Length of output: 471
Default value mismatch between component definition and API documentation.
The component sets default: true for headerDragable, but the API documentation in examples/sites/demos/apis/modal.js (line 129) specifies defaultValue: 'false'. The demo in modal-header.vue explicitly sets headerDragable: false in both the template and function call, which corroborates that the intended default is true (dragging enabled by default).
Update the API documentation to match the component definition:
- defaultValue: 'false',
+ defaultValue: 'true',Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/vue/src/modal/src/index.ts around line 99 the prop headerDragable is
defined with default: true but the API docs in
examples/sites/demos/apis/modal.js declare defaultValue: 'false'; update the API
documentation to match the component by changing the defaultValue in
examples/sites/demos/apis/modal.js (near line 129) to 'true' (or the boolean
true representation used in that file) so the docs reflect the actual component
default.
| { | ||
| name: 'header-dragable', | ||
| type: 'boolean', | ||
| defaultValue: 'false', |
There was a problem hiding this comment.
你好，这里的默认值应该是true，还有新加的特性需要补充下版本信息哈
2 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?
不支持设置 headerDragable 属性
Issue Number: N/A
What is the new behavior?
支持设置 headerDragable 属性
Does this PR introduce a breaking change?
Other information
其实这里 Dragable 是个错别字，为了和旧 API
footerDragale风格保持一致，将错就错。Summary by CodeRabbit
New Features
header-dragableoption to the modal component, allowing control over whether the modal header can be dragged (enabled by default).Changes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.