fix(ui): Respect safe area for fullscreen media #2456
Conversation
This change ensures that the fullscreen media viewer on both mobile and desktop properly respects the safe area insets (like notches and system bars). It replaces the `AnimatedPadding` widget with padding on the `AnimatedContainer` itself, using `MediaQuery.paddingOf(context)` to correctly calculate the top and bottom padding. This prevents the media content from being obscured by system UI elements when the details (header/footer) are visible.
WalkthroughThis PR addresses issue Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
packages/stream_chat_flutter/CHANGELOG.md(1 hunks)packages/stream_chat_flutter/lib/src/fullscreen_media/full_screen_media.dart(1 hunks)packages/stream_chat_flutter/lib/src/fullscreen_media/full_screen_media_desktop.dart(1 hunks)sample_app/ios/Runner.xcodeproj/project.pbxproj(0 hunks)
💤 Files with no reviewable changes (1)
- sample_app/ios/Runner.xcodeproj/project.pbxproj
⏰ 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). (9)
- GitHub Check: stream_chat_persistence
- GitHub Check: stream_chat
- GitHub Check: stream_chat_localizations
- GitHub Check: stream_chat_flutter_core
- GitHub Check: stream_chat_flutter
- GitHub Check: analyze_legacy_versions
- GitHub Check: build (ios)
- GitHub Check: test
- GitHub Check: build (android)
🔇 Additional comments (5)
packages/stream_chat_flutter/lib/src/fullscreen_media/full_screen_media_desktop.dart (2)
320-331: Padding is applied unconditionally, but should it vary based onisDisplayingDetail?The padding is always applied with
padding.top + kToolbarHeightandpadding.bottom + kToolbarHeight, regardless of whether the header/footer are visible (isDisplayingDetail). WhenisDisplayingDetailisfalse, the header and footer slide off-screen, so applying full padding would leave unnecessary empty space around the media content.Consider making the padding conditional:
return AnimatedContainer( duration: kThemeChangeDuration, color: switch (isDisplayingDetail) { true => StreamChannelHeaderTheme.of(context).color, false => Colors.black, }, - padding: EdgeInsetsDirectional.only( - top: padding.top + kToolbarHeight, - bottom: padding.bottom + kToolbarHeight, - ), + padding: isDisplayingDetail + ? EdgeInsetsDirectional.only( + top: padding.top + kToolbarHeight, + bottom: padding.bottom + kToolbarHeight, + ) + : EdgeInsets.zero, child: child, );Please verify whether the padding should animate away when the header/footer are hidden. The current implementation always reserves space for the toolbar even when it's not visible.
335-383: Good refactoring to separate content rendering from the animated container.The use of a
Builderchild to handle attachment-type rendering is clean and improves readability. The structure properly separates the animation/padding concerns from the actual media rendering logic.packages/stream_chat_flutter/lib/src/fullscreen_media/full_screen_media.dart (2)
284-295: Same concern: padding is applied unconditionally regardless of header/footer visibility.Similar to the desktop version, the padding is always
padding.top + kToolbarHeightandpadding.bottom + kToolbarHeight, even whenisDisplayingDetailisfalseand the header/footer have animated off-screen.Consider making the padding conditional to avoid unnecessary empty space when the UI chrome is hidden:
return AnimatedContainer( duration: kThemeChangeDuration, color: switch (isDisplayingDetail) { true => StreamChannelHeaderTheme.of(context).color, false => Colors.black, }, - padding: EdgeInsetsDirectional.only( - top: padding.top + kToolbarHeight, - bottom: padding.bottom + kToolbarHeight, - ), + padding: isDisplayingDetail + ? EdgeInsetsDirectional.only( + top: padding.top + kToolbarHeight, + bottom: padding.bottom + kToolbarHeight, + ) + : EdgeInsets.zero, child: child, );Please verify whether the intended UX is to keep the safe area padding visible at all times, or to animate it away with the header/footer. If the former, disregard this comment.
299-330: LGTM!The
Builderchild pattern is consistent with the desktop variant. Video initialization is properly handled ininitState, and the loading indicator provides good UX while the video controller initializes.packages/stream_chat_flutter/CHANGELOG.md (1)
1-7: LGTM!The changelog entry correctly documents the fix and references the linked issue #2454. The format is consistent with existing entries.
packages/stream_chat_flutter/lib/src/fullscreen_media/full_screen_media_desktop.dart
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2456 +/- ##
=======================================
Coverage 64.60% 64.61%
=======================================
Files 420 420
Lines 26210 26210
=======================================
+ Hits 16934 16936 +2
+ Misses 9276 9274 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Submit a pull request
Fixes: #2454
Description of the pull request
This change ensures that the fullscreen media viewer on both mobile and desktop properly respects the safe area insets (like notches and system bars).
It replaces the
AnimatedPaddingwidget with padding on theAnimatedContaineritself, usingMediaQuery.paddingOf(context)to correctly calculate the top and bottom padding. This prevents the media content from being obscured by system UI elements when the details (header/footer) are visible.Screenshots / Videos
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.