#449 feat/ add fullscreen mode for photos in detail views #488
Conversation
- added const for images in detail view
There was a problem hiding this comment.
Really god work here! Nice and clean. I left few suggestions and questions of mine.
Regarding problem you described in PR description, maybe it has sth to do with image size?
@simon-the-shark if you have free time please look at it as well. Especially on part where navigator comes to play.
| static const heightImage = 254.0; | ||
| static const double spacerHeight = 16; |
There was a problem hiding this comment.
please specify the type of heightImage, imo better to rename it to: imageHeight (just to follow convention)
| const SizedBox(height: DetailViewsConfig.heightImage), | ||
| SizedBox( | ||
| height: DetailViewsConfig.heightImage, | ||
| child: ZoomableOptimizedDirectusImage( | ||
| digitalGuideResponseExtended.imageUrl, | ||
| ), |
There was a problem hiding this comment.
Is SizedBox declared twice needed here?
| @override | ||
| Widget build(BuildContext context) { | ||
| return Align( | ||
| alignment: Alignment.bottomCenter, | ||
| child: SizedBox.square( | ||
| child: ListView( | ||
| child: SingleChildScrollView( |
There was a problem hiding this comment.
why SingleChildScrollView here?
| Size _getImagesSize(BuildContext context, BoxConstraints constraints) { | ||
| final pixelsDensity = MediaQuery.devicePixelRatioOf(context); | ||
| return Size( | ||
| constraints.maxWidth * pixelsDensity, | ||
| constraints.maxHeight * pixelsDensity, | ||
| ); | ||
| } |
There was a problem hiding this comment.
imo an overkill here. I'd live it as I was before
| Future<void> _showFullScreenImage(BuildContext context) async { | ||
| await Navigator.of(context).push( | ||
| MaterialPageRoute( | ||
| builder: (newContext) => Scaffold( |
There was a problem hiding this comment.
using newContext instead of just context can lead to mistakes
There was a problem hiding this comment.
the convention is just to use context always
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
Files not reviewed (7)
- lib/config/ui_config.dart: Language not supported
- lib/features/digital_guide_view/general_info/presentation/digital_guide_view.dart: Language not supported
- lib/features/guide_detail_view/guide_detail_view.dart: Language not supported
- lib/widgets/detail_views/sliver_header_section.dart: Language not supported
- lib/widgets/detail_views/sliver_logo.dart: Language not supported
- lib/widgets/optimized_directus_image.dart: Language not supported
- lib/widgets/zoomable_optimized_directus_image.dart: Language not supported
Okay so, generally this works pretty nice. Although I have a problem that when the zoomable image will be shown in fullscreen, in some cases the Scaffold will only contain the image leaving black areas around it. I tried many approaches but i can't get it to work...
Correct scafold coverage:
Incorrect scaffold coverage (there are black areas above and under the image):
Notice that here there is an image with transparency, there is a greyLight color underneath it.