fix(dialog-box): [dialog-box] add aria-* attribute #3952
Conversation
WalkthroughTests were scoped to demo containers; dialog component gains semantic ARIA attributes and renderless state now generates stable Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
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: 1
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
examples/sites/demos/pc/app/dialog-box/basic-usage.spec.tsexamples/sites/demos/pc/app/dialog-box/form-in-dialog.spec.tspackages/vue/src/dialog-box/src/pc.vue
⏰ 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 (2)
examples/sites/demos/pc/app/dialog-box/form-in-dialog.spec.ts (1)
7-7: LGTM! Selector scoping improves test isolation.Scoping the dialogBox locator to the demo container instead of the entire page is a good practice that prevents conflicts when multiple demos exist on the same page.
examples/sites/demos/pc/app/dialog-box/basic-usage.spec.ts (1)
8-24: LGTM! Consistent selector scoping across all interactions.All button interactions are now properly scoped to the demo container, which improves test isolation and prevents conflicts when multiple demos exist on the same page. The test logic and assertions remain unchanged.
| role="dialog" | ||
| :aria-modal="true" | ||
| aria-labelledby="tiny_dialog_unique_0" | ||
| aria-describedby="tiny_dialog_unique_1" |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @packages/renderless/src/dialog-box/vue.ts:
- Line 87: The new dragStyle: null property in dialog-box/vue.ts is unrelated to
the ARIA changes and should either be removed or justified: if it was
accidentally mixed in, delete the line and revert any corresponding type
additions; if it’s intentional, add a short inline comment explaining its
purpose, update the component’s props/type definitions (e.g., the dialog
component’s props/interface) to include dragStyle, and mention it in the PR
description so reviewers know it’s a deliberate change. Ensure consistency
between the implementation, types, and PR text.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/renderless/src/dialog-box/vue.tspackages/renderless/types/dialog-box.type.tspackages/vue/src/dialog-box/src/pc.vue
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/vue/src/dialog-box/src/pc.vue
⏰ 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 (3)
packages/renderless/types/dialog-box.type.ts (1)
64-65: LGTM! Type definitions for ARIA identifiers.The addition of
titleIdandcontentIdto the state interface is appropriate for supporting ARIA attributes (aria-labelledbyandaria-describedby) in the dialog component.packages/renderless/src/dialog-box/vue.ts (2)
88-89: Good: Unique IDs generated for ARIA attributes.The
titleIdandcontentIdgeneration with unique identifiers and descriptive prefixes is appropriate for ARIA attribute support. The 8-character nanoid length should provide sufficient uniqueness for dialog instances on a page.However, please verify the nanoid usage pattern
nanoid.api.nanoid(8)is correct (see comment on line 34).
34-34: The nanoid import and usage pattern is correct. The@opentiny/utilspackage exports nanoid as a namespace module containing anapiobject with thenanoidfunction, makingnanoid.api.nanoid(8)the proper way to access it. No changes needed.Likely an incorrect or invalid review comment.
| animationName: computed(() => api.computedAnimationName()), | ||
| current, | ||
| dragStyle: null | ||
| dragStyle: null, |
There was a problem hiding this comment.
Clarify the unrelated dragStyle property addition.
The addition of dragStyle: null appears unrelated to the PR objective of adding ARIA attributes. This property is not mentioned in the PR description, AI summary, or type definitions changes. Please confirm whether this is:
- An intentional fix that should be documented
- Part of another feature that got mixed in
- A necessary property that was previously missing
🤖 Prompt for AI Agents
In @packages/renderless/src/dialog-box/vue.ts at line 87, The new dragStyle:
null property in dialog-box/vue.ts is unrelated to the ARIA changes and should
either be removed or justified: if it was accidentally mixed in, delete the line
and revert any corresponding type additions; if it’s intentional, add a short
inline comment explaining its purpose, update the component’s props/type
definitions (e.g., the dialog component’s props/interface) to include dragStyle,
and mention it in the PR description so reviewers know it’s a deliberate change.
Ensure consistency between the implementation, types, and PR text.
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
Accessibility Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.