-
Notifications
You must be signed in to change notification settings - Fork 282
Fix: prevent InfoDialog from flickering to blue variant on close #630
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Fix: prevent InfoDialog from flickering to blue variant on close #630
Conversation
WalkthroughModified the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/src/features/infoDialogSlice.ts (2)
35-35: Prefer nullish coalescing (??) over logical OR (||).While
||works for this use case,??is more precise in TypeScript as it only defaults when the value isnullorundefined, not when it's any falsy value. Given thatInfoDialogVariantis likely a union of non-empty string literals, this distinction matters for type safety.Apply this diff:
- state.variant = state.variant || 'info'; + state.variant = state.variant ?? 'info';
31-37: Consider deferring state cleanup until after the close animation completes.The current approach clears
titleandmessageimmediately while preservingvariantto prevent the flicker. This creates inconsistent state handling—some fields are cleared while others are preserved. While this fixes the immediate symptom, a more robust architectural solution would be to preserve the entire dialog state until the closing animation finishes, then clear everything. This could be achieved by:
- Using an animation callback (e.g.,
onAnimationEnd) to dispatch state cleanup- Introducing a separate "closing" state distinct from "closed"
- Letting the component handle state cleanup after animation
The current fix works but may be fragile if other fields (title, message) are also rendered during the close animation.
Closes #623
Addresses the flickering issue in the InfoDialog component, where the dialog momentarily switches to the blue variant upon closing an error dialog.
This update begins refactoring the dialog state handling logic to ensure consistent visual feedback and transition behaviour.
Testing Videos before and after changes :
Before :
before_update.mov
After:
after_update.mov
Summary by CodeRabbit