-
-
Notifications
You must be signed in to change notification settings - Fork 287
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
feat: 3129 (New PR) Allow to swipe between product images on the full screen image #3363
feat: 3129 (New PR) Allow to swipe between product images on the full screen image #3363
Conversation
…into swipeable_product_images_view
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.
Hi @omkarChend1kar!
Please fix the warnings first:
info • Unused import: 'package:smooth_app/generic_lib/design_constants.dart' • packages/smooth_app/lib/pages/product/product_image_viewer.dart:15:8 • unused_import
info • Unused import: 'package:smooth_app/generic_lib/widgets/smooth_back_button.dart' • packages/smooth_app/lib/pages/product/product_image_viewer.dart:18:8 • unused_import
warning • The left operand can't be null, so the right operand is never executed • packages/smooth_app/lib/pages/product/product_image_viewer.dart:105:27 • dead_null_aware_expression
Yes, On it. |
Codecov Report
@@ Coverage Diff @@
## develop #3363 +/- ##
===========================================
- Coverage 10.47% 10.43% -0.04%
===========================================
Files 255 256 +1
Lines 12421 12463 +42
===========================================
Hits 1301 1301
- Misses 11120 11162 +42
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
Looks good @omkarChend1kar!
Again, the use of ValueNotifier
instead of setState
is a quite unexpected, so do add a comment explaining that you use it for performance reasons.
And then we're ok (except maybe for initialProductImageCategoryIndex
that could be shortened to initialImageIndex
, but that's a detail).
packages/smooth_app/lib/pages/product/product_image_viewer.dart
Outdated
Show resolved
Hide resolved
…karChend1kar/smooth-app into swipeable_product_images_view
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.
That's great, thank you @omkarChend1kar!
Except for the format. Please have a look at it again. |
Yeah |
Yeah. Thanks for being supportive @monsieurtanuki! |
What
Screenshot/Mockups after :
swiping.mp4
Part of