fix: alert dialog styles and remove built-in close button#813
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR removes the explicit Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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.
🧹 Nitpick comments (1)
packages/raystack/components/alert-dialog/__tests__/alert-dialog.test.tsx (1)
93-123: ⚡ Quick winAdd a regression test for removed built-in close button.
Please add an assertion that the default
"Close dialog"button is not rendered after opening, so the API-removal contract is locked in by tests.Proposed test addition
describe('Close Behavior', () => { + it('does not render a built-in close button', async () => { + renderAndOpenAlertDialog(<BasicAlertDialog />); + + await waitFor(() => { + expect(screen.getByRole('alertdialog')).toBeInTheDocument(); + }); + + expect( + screen.queryByRole('button', { name: /close dialog/i }) + ).not.toBeInTheDocument(); + }); + it('closes when AlertDialog.Close is clicked', async () => { renderAndOpenAlertDialog(<BasicAlertDialog />);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/raystack/components/alert-dialog/__tests__/alert-dialog.test.tsx` around lines 93 - 123, Add an assertion to the Close Behavior tests to verify the removed built-in close button is not present: after calling renderAndOpenAlertDialog(<BasicAlertDialog />) and confirming the alertdialog appears (using screen.getByRole('alertdialog')), assert that a button with accessible name "Close dialog" is not in the document (e.g., expect(screen.queryByRole('button', { name: /Close dialog/i })).toBeNull() or similar). Place this check in the same 'closes when AlertDialog.Close is clicked' or 'does not close on outside click' test blocks (or a new test) to lock the API-removal contract; reference the existing helpers and symbols renderAndOpenAlertDialog, BasicAlertDialog, ALERT_CLOSE and styles.dialogOverlay when locating where to add the assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/raystack/components/alert-dialog/__tests__/alert-dialog.test.tsx`:
- Around line 93-123: Add an assertion to the Close Behavior tests to verify the
removed built-in close button is not present: after calling
renderAndOpenAlertDialog(<BasicAlertDialog />) and confirming the alertdialog
appears (using screen.getByRole('alertdialog')), assert that a button with
accessible name "Close dialog" is not in the document (e.g.,
expect(screen.queryByRole('button', { name: /Close dialog/i })).toBeNull() or
similar). Place this check in the same 'closes when AlertDialog.Close is
clicked' or 'does not close on outside click' test blocks (or a new test) to
lock the API-removal contract; reference the existing helpers and symbols
renderAndOpenAlertDialog, BasicAlertDialog, ALERT_CLOSE and styles.dialogOverlay
when locating where to add the assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e31c553e-4861-47aa-aede-cda64afbd79d
📒 Files selected for processing (11)
apps/www/src/components/playground/alert-dialog-examples.tsxapps/www/src/content/docs/components/alert-dialog/demo.tsapps/www/src/content/docs/components/alert-dialog/index.mdxapps/www/src/content/docs/components/alert-dialog/props.tspackages/raystack/components/alert-dialog/__tests__/alert-dialog.test.tsxpackages/raystack/components/alert-dialog/alert-dialog-content.tsxpackages/raystack/components/alert-dialog/alert-dialog-misc.tsxpackages/raystack/components/alert-dialog/alert-dialog.module.csspackages/raystack/components/alert-dialog/alert-dialog.tsxpackages/raystack/components/dialog/dialog-misc.tsxpackages/raystack/components/dialog/dialog.module.css
💤 Files with no reviewable changes (2)
- apps/www/src/content/docs/components/alert-dialog/props.ts
- packages/raystack/components/alert-dialog/alert-dialog-content.tsx
2 participants
Summary
AlertDialogspacing: header getsspace-7on all sides (collapses tospace-5bottom when followed by a body); body is0 / space-7 / space-5(orspace-7all sides when no header precedes it); removed header/footer borders.AlertDialog.Headeris nowdirection='column'withgap={3}soTitle+Descriptionstack naturally inside it; docs example updated to use the header (noBodywrapper).AlertDialog: removed theshowCloseButtonprop, the internalCloseButton, and the correspondingAlertDialogCloseButtonProps/docs section. Updated examples and tests accordingly.AlertDialogdocs examples.Dialogtweaks: close button now renders viaIconButton, default content width pinned to 400px, body padding bumped tospace-9 / space-7.