feat(modal): [modal] add show-close feature #2840
Conversation
WalkthroughThe pull request introduces a new Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
WalkthroughThis pull request introduces a new feature to the modal component, allowing users to toggle the visibility of the close button. The default behavior is set to show the close button. The changes include updates to the demo examples and the core modal component to support this feature. Changes
|
![petercat-assistant[bot]](https://avatars.githubusercontent.com/in/875736?s=60&v=4){kind=small}
| @@ -335,7 +335,11 @@ export default { | |||
| 'zh-CN': '是否显示关闭按钮，默认值为 true', | |||
| 'en-US': '' | |||
There was a problem hiding this comment.
The English description for the show-close feature is missing. Please ensure to provide a complete description for consistency across languages.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/vue/src/modal/src/pc.vue (1)
Test coverage needed for modal close button functionality
The codebase lacks proper test coverage for the close button feature. Please add tests for:
- Default visibility of the close button (should be visible by default)
- Hiding close button when
showCloseis set tofalse- Message type modal's default behavior (close button should be hidden)
- Interaction between
showCloseandmessageClosablepropsSuggested test location:
examples/sites/demos/pc/app/modal/modal-header.spec.ts🔗 Analysis chain
Line range hint
92-222: Verify test coverage for the show-close feature.The PR objectives mention that tests haven't been added. Please ensure test coverage for:
- Default behavior of the close button
- Hiding the close button when
showCloseis false- Interaction between
showCloseandmessageClosableprops🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for existing tests and potential test locations echo "Searching for modal test files..." fd -e spec.js -e spec.ts -e test.js -e test.ts modal echo "Checking test coverage in existing modal tests..." rg -l "showClose|show-close" $(fd -e spec.js -e spec.ts -e test.js -e test.ts modal)Length of output: 1781
Script:
#!/bin/bash # Check content of relevant test files and implementation echo "=== Modal Header Tests ===" cat examples/sites/demos/pc/app/modal/modal-header.spec.ts echo -e "\n=== Modal Other Tests ===" cat examples/sites/demos/pc/app/modal/modal-other.spec.ts echo -e "\n=== Modal Implementation ===" rg -A 5 "showClose.*:" packages/vue/src/modal/Length of output: 2324
🧹 Nitpick comments (2)
examples/sites/demos/pc/app/modal/modal-other-composition-api.vue (1)
19-19: LGTM! Consider documenting the show-close behavior.The
show-closeprop is correctly implemented in the template-based modal.Consider adding a comment to explain why the close button behavior differs between template-based and function-based modals in this demo.
packages/vue/src/modal/src/pc.vue (1)
215-222: Add ARIA attributes for better accessibility.While the close button implementation is correct, consider adding ARIA attributes to improve accessibility.
showClose ? h(iconClose(), { class: ['tiny-modal__close-btn', 'trigger__btn'], + attrs: { + 'aria-label': t('ui.modal.close'), + role: 'button', + tabindex: '0' + }, on: { click: this.closeEvent } }) : null
📜 Review details
Configuration used: CodeRabbit 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-other-composition-api.vue(2 hunks)examples/sites/demos/pc/app/modal/modal-other.vue(2 hunks)examples/sites/demos/pc/app/modal/webdoc/modal.js(1 hunks)packages/vue/src/modal/src/pc.vue(3 hunks)
🔇 Additional comments (5)
examples/sites/demos/pc/app/modal/modal-other-composition-api.vue (1)
62-62: LGTM! Consistent with the feature implementation.The
showClose: falsesetting demonstrates the ability to hide the close button in function-based modals.examples/sites/demos/pc/app/modal/modal-other.vue (1)
19-19: LGTM! Implementation matches the Composition API version.The show-close feature is implemented consistently between the Options API and Composition API versions of the demo.
Also applies to: 66-66
examples/sites/demos/pc/app/modal/webdoc/modal.js (1)
206-207: LGTM! Documentation is clear and bilingual.The show-close feature is well documented in both Chinese and English, with the default value correctly specified.
Also applies to: 212-213
examples/sites/demos/apis/modal.js (1)
338-342: LGTM! API definition is complete and properly versioned.The show-close feature is:
- Properly versioned with stable version 3.22.0
- Correctly supported in both PC and mobile-first modes
- Linked to the appropriate demo
packages/vue/src/modal/src/pc.vue (1)
92-104: LGTM! Clean destructuring of props.The destructuring of the
showCloseprop alongside other modal properties is well-organized and follows the component's existing patterns.
| 'slots', | ||
| 'showClose' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type definition and default value for the showClose prop.
The new showClose prop should include type information and a default value for better maintainability and developer experience.
- 'slots',
- 'showClose'
+ 'slots',
+ {
+ name: 'showClose',
+ type: Boolean,
+ default: true,
+ description: 'Controls the visibility of the modal close button'
+ }Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
examples/sites/demos/pc/app/modal/modal-other.spec.ts (2)
5-5: Consider using more stable selectors for E2E tests.The current CSS selector
.tiny-modal__close-btn > .tiny-svgis tightly coupled to implementation details. Consider using more stable selectors likedata-testidattributes or ARIA roles for better test maintainability.- const close = page.locator('.tiny-modal__close-btn > .tiny-svg') + const close = page.locator('[data-testid="modal-close-button"]')
Line range hint
3-12: Consider internationalizing test descriptions and labels.The test descriptions and button labels are in Chinese while the codebase appears to be in English. Consider using English for consistency or implementing proper i18n test utilities.
-test('弹窗其他特性', async ({ page }) => { +test('modal other features', async ({ page }) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
examples/sites/demos/pc/app/modal/modal-other.spec.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: PR E2E Test (pnpm test:e2e3)
| await page.goto('modal#modal-other') | ||
| await page.getByRole('button', { name: '弹窗 esc 关闭' }).first().click() | ||
| await expect(close).toBeHidden() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for show-close feature.
The test only verifies that the close button is hidden. Consider adding assertions to verify:
- The close button is initially visible when show-close is true
- The close button visibility toggles correctly with show-close prop changes
James-9696
changed the title
PR
新增modal组件是否显示关闭按钮新特性
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: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
New Features
show-closeproperty to the modal component, allowing customization of the close button visibility.Documentation
show-closeproperty.3.22.0for the feature.