feat(alert): [alert] display title when size is large #2744
Conversation
WalkthroughThis pull request introduces a new Changes
Possibly related PRs
Suggested reviewers
Poem
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 PR introduces a new feature to the alert component, allowing the display of a title when the alert size is set to large. A new property Changes
|
![petercat-assistant[bot]](https://avatars.githubusercontent.com/in/875736?s=60&v=4){kind=small}
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
examples/sites/demos/apis/alert.js(1 hunks)examples/sites/demos/pc/app/alert/title-composition-api.vue(1 hunks)examples/sites/demos/pc/app/alert/title.vue(1 hunks)examples/sites/demos/pc/app/alert/webdoc/alert.js(1 hunks)packages/theme/src/alert/index.less(1 hunks)packages/vue/src/alert/src/index.ts(1 hunks)packages/vue/src/alert/src/pc.vue(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: PR E2E Test (pnpm test:e2e3)
🔇 Additional comments (7)
packages/vue/src/alert/src/index.ts (1)
88-91: Consider verifying default behavior and test coverage for the new prop.
The newshowTitleprop is a great addition. With a default value oftrue, it's important to confirm that scenarios requiring no title are tested. Also, ensure consistency between the PascalCase (showTitle) in the underlying code and the kebab-case (show-title) in component usage.examples/sites/demos/pc/app/alert/webdoc/alert.js (1)
56-56: Documentation update approved.
Your documentation expansion clarifies how theshow-titleattribute controls title visibility when the size islarge. This aligns with the introduced property, improving discoverability.examples/sites/demos/apis/alert.js (1)
121-134: Added property looks consistent.
Theshow-titleproperty with its default value and meta version is coherent and well-documented. It complements thesizeproperty logic. Please confirm that demos cover bothtrueandfalsescenarios.examples/sites/demos/pc/app/alert/title-composition-api.vue (1)
9-9: Good example usage.
Explicitly setting:show-title="false"demonstrates disabling the title for large sized alerts. This showcases the new property’s flexibility in practice.examples/sites/demos/pc/app/alert/title.vue (1)
9-9: LGTM! Good demo coverage of the new feature.The new example effectively demonstrates how to hide the title using
:show-title="false"while maintaining size="large". This complements the existing examples well.packages/vue/src/alert/src/pc.vue (1)
20-21: LGTM! Clean implementation of conditional title display.The template logic correctly combines the size check with the new showTitle prop.
packages/theme/src/alert/index.less (1)
57-58: LGTM! Consistent margin implementation.The margin-right addition for the description follows the same pattern as the title, preventing overlap with the close icon. Good use of existing CSS variables for consistency.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
examples/sites/demos/pc/app/alert/title.spec.ts (1)
13-14: Consider enhancing test coverage for show-title property.While the test verifies the absence of "消息" text, it would be more explicit to directly test the
show-titleproperty's functionality.Consider adding these assertions:
const alert1 = page.locator('.tiny-alert--large').nth(2) await expect(alert1).not.toHaveText('消息') + const title1 = alert1.locator('.tiny-alert__title') + await expect(title1).toHaveCount(0)This would explicitly verify that the title element is not present when
show-titleis false.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
examples/sites/demos/pc/app/alert/title.spec.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: PR Unit Test
- GitHub Check: PR E2E Test (pnpm test:e2e3)
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: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
New Features
show-titleproperty to the Alert component.Bug Fixes
Documentation
show-titleattribute functionality.Tests