feat(modal): supports setting the headerDragable property #3918
Conversation
WalkthroughAdds a new headerDragable prop to the modal (core default true), conditions header mousedown binding, updates API docs and demos, and sets the API 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)
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ 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)
🔇 Additional comments (3)
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.
| <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.
examples/sites/demos/apis/modal.js
Outdated
| { | ||
| name: 'header-dragable', | ||
| type: 'boolean', | ||
| defaultValue: 'false', |
There was a problem hiding this comment.
你好，这里的默认值应该是true，还有新加的特性需要补充下版本信息哈
gausszhou
force-pushed
the
feature/modal_header
branch
from
35e9403 to
efe2045
Compare
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
Changes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.