Skip to content
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

Conversation

omkarChend1kar
Copy link
Contributor

@omkarChend1kar omkarChend1kar requested a review from a team as a code owner November 27, 2022 07:46
@omkarChend1kar omkarChend1kar changed the title feat : 3129 (New PR) Allow to swipe between product images on the full screen image feat: 3129 (New PR) Allow to swipe between product images on the full screen image Nov 27, 2022
Copy link
Contributor

@monsieurtanuki monsieurtanuki left a 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

@omkarChend1kar
Copy link
Contributor Author

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-commenter
Copy link

codecov-commenter commented Nov 27, 2022

Codecov Report

Merging #3363 (a8c2e08) into develop (2621b3a) will decrease coverage by 0.03%.
The diff coverage is 0.00%.

@@             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     
Impacted Files Coverage Δ
..._lib/widgets/images/smooth_images_sliver_list.dart 0.00% <0.00%> (ø)
...generic_lib/widgets/images/smooth_images_view.dart 0.00% <ø> (ø)
.../lib/pages/product/product_image_gallery_view.dart 0.00% <0.00%> (ø)
...ib/pages/product/product_image_swipeable_view.dart 0.00% <0.00%> (ø)
...th_app/lib/pages/product/product_image_viewer.dart 0.00% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@monsieurtanuki monsieurtanuki left a 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).

Copy link
Contributor

@monsieurtanuki monsieurtanuki left a 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!

@monsieurtanuki
Copy link
Contributor

Except for the format. Please have a look at it again.

@omkarChend1kar
Copy link
Contributor Author

Except for the format. Please have a look at it again.

Yeah

@omkarChend1kar
Copy link
Contributor Author

That's great, thank you @omkarChend1kar!

Yeah. Thanks for being supportive @monsieurtanuki!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants