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.

Sign up for GitHub

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

Jump to bottom

fix: image edit and not logged-in user #3689

Merged
merged 3 commits into from
Feb 9, 2023
Merged

fix: image edit and not logged-in user #3689

merged 3 commits into from
Feb 9, 2023

Conversation

Akashsri3bi
Copy link
Contributor

Though to develop on existing function , It worked well on debug .

@Akashsri3bi Akashsri3bi requested a review from a team as a code owner February 9, 2023 10:25
monsieurtanuki
monsieurtanuki reviewed Feb 9, 2023
View reviewed changes
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.

Thank you @Akashsri3bi, that part is perfect!

Now what needs to be changed is the access to the product images page, that should not be restricted.
cf. issue:

  1. Go to a product page
  2. Click on the "edit" button
  3. You land on the "edit product" page
  4. There's a "edit photo" item, but if you click it you get a "for that feature you must be connected" message

Now on step 4 you should be able to click on edit photo and land on all photo pages, which gives access to (almost) full-screen photo. And then to the unselect/crop/camera/gallery edit photo buttons, where you've just put the "check log-in" restriction in your PR. OK?

@monsieurtanuki
Copy link
Contributor

@Akashsri3bi And you have to fix the warnings too:

Analyzing smooth-app...                                         

   info • Sort directive sections alphabetically • packages/smooth_app/lib/pages/product/product_image_viewer.dart:25:1 • directives_ordering
   info • Don't use 'BuildContext's across async gaps • packages/smooth_app/lib/pages/product/product_image_viewer.dart:179:16 • use_build_context_synchronously
   info • Don't use 'BuildContext's across async gaps • packages/smooth_app/lib/pages/product/product_image_viewer.dart:191:16 • use_build_context_synchronously
   info • Don't use 'BuildContext's across async gaps • packages/smooth_app/lib/pages/product/product_image_viewer.dart:194:37 • use_build_context_synchronously
   info • Don't use 'BuildContext's across async gaps • packages/smooth_app/lib/pages/product/product_image_viewer.dart:237:16 • use_build_context_synchronously
   info • Don't use 'BuildContext's across async gaps • packages/smooth_app/lib/pages/product/product_image_viewer.dart:261:23 • use_build_context_synchronously
   info • Don't use 'BuildContext's across async gaps • packages/smooth_app/lib/pages/product/product_image_viewer.dart:275:[16](https://github.com/openfoodfacts/smooth-app/actions/runs/4133284653/jobs/7143001943#step:7:17) • use_build_context_synchronously

@Akashsri3bi
Copy link
Contributor Author

Thank you @Akashsri3bi, that part is perfect!

Now what needs to be changed is the access to the product images page, that should not be restricted. cf. issue:

  1. Go to a product page
  2. Click on the "edit" button
  3. You land on the "edit product" page
  4. There's a "edit photo" item, but if you click it you get a "for that feature you must be connected" message

Now on step 4 you should be able to click on edit photo and land on all photo pages, which gives access to (almost) full-screen photo. And then to the unselect/crop/camera/gallery edit photo buttons, where you've just put the "check log-in" restriction in your PR. OK?

Gotcha!

@Akashsri3bi
Copy link
Contributor Author

@Akashsri3bi And you have to fix the warnings too:

Analyzing smooth-app...                                         

   info • Sort directive sections alphabetically • packages/smooth_app/lib/pages/product/product_image_viewer.dart:25:1 • directives_ordering
   info • Don't use 'BuildContext's across async gaps • packages/smooth_app/lib/pages/product/product_image_viewer.dart:179:16 • use_build_context_synchronously
   info • Don't use 'BuildContext's across async gaps • packages/smooth_app/lib/pages/product/product_image_viewer.dart:191:16 • use_build_context_synchronously
   info • Don't use 'BuildContext's across async gaps • packages/smooth_app/lib/pages/product/product_image_viewer.dart:194:37 • use_build_context_synchronously
   info • Don't use 'BuildContext's across async gaps • packages/smooth_app/lib/pages/product/product_image_viewer.dart:237:16 • use_build_context_synchronously
   info • Don't use 'BuildContext's across async gaps • packages/smooth_app/lib/pages/product/product_image_viewer.dart:261:23 • use_build_context_synchronously
   info • Don't use 'BuildContext's across async gaps • packages/smooth_app/lib/pages/product/product_image_viewer.dart:275:[16](https://github.com/openfoodfacts/smooth-app/actions/runs/4133284653/jobs/7143001943#step:7:17) • use_build_context_synchronously

I tried to look it It seems to be having more issues do you have any tips around . this didn't help [(https://stackoverflow.com/questions/68871880/do-not-use-buildcontexts-across-async-gaps)]

@monsieurtanuki
Copy link
Contributor

I tried to look it It seems to be having more issues do you have any tips around . this didn't help [(https://stackoverflow.com/questions/68871880/do-not-use-buildcontexts-across-async-gaps)]

What about https://stackoverflow.com/a/69253529/14517119 and context.mounted?

@Akashsri3bi
Copy link
Contributor Author

I tried to look it It seems to be having more issues do you have any tips around . this didn't help [(https://stackoverflow.com/questions/68871880/do-not-use-buildcontexts-across-async-gaps)]

What about https://stackoverflow.com/a/69253529/14517119 and context.mounted?

I tried to look it It seems to be having more issues do you have any tips around . this didn't help [(https://stackoverflow.com/questions/68871880/do-not-use-buildcontexts-across-async-gaps)]

What about https://stackoverflow.com/a/69253529/14517119 and context.mounted?

seems like context.mounted doesn't work in this case it disturbs other calls

@codecov-commenter
Copy link

Codecov Report

Merging #3689 (01a0d2a) into develop (ad46236) will decrease coverage by 0.11%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           develop   #3689      +/-   ##
==========================================
- Coverage     9.45%   9.34%   -0.11%     
==========================================
  Files          269     272       +3     
  Lines        13571   13722     +151     
==========================================
  Hits          1283    1283              
- Misses       12288   12439     +151
Impacted Files Coverage Δ
...mooth_app/lib/pages/product/edit_product_page.dart 0.50% <ø> (+<0.01%) ⬆️
...th_app/lib/pages/product/product_image_viewer.dart 0.00% <0.00%> (ø)
packages/smooth_app/lib/main.dart 13.70% <0.00%> (-1.34%) ⬇️
...ib/pages/preferences/user_preferences_connect.dart 5.50% <0.00%> (-1.32%) ⬇️
...s/smooth_app/lib/tmp_crop_image/new_crop_page.dart 0.00% <0.00%> (ø)
...smooth_app/lib/pages/product/new_product_page.dart 0.00% <0.00%> (ø)
...ooth_app/lib/background/background_task_image.dart 0.00% <0.00%> (ø)
...oth_app/lib/pages/product/edit_new_packagings.dart 0.00% <0.00%> (ø)
...oth_app/lib/pages/product/simple_input_widget.dart 0.00% <0.00%> (ø)
...oth_app/lib/tmp_crop_image/rotated_crop_image.dart 0.00% <0.00%> (ø)
... and 8 more

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

monsieurtanuki
monsieurtanuki approved these changes Feb 9, 2023
View reviewed changes
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 to me @Akashsri3bi!

The part about ignore: use_build_context_synchronously is not very important but should be fixed one day, and that's easy with flutter 3.7 (#1815).

@monsieurtanuki monsieurtanuki merged commit 1c1acd8 into openfoodfacts:develop Feb 9, 2023
@github-actions github-actions bot mentioned this pull request Feb 9, 2023
Merged
@Akashsri3bi
Copy link
Contributor Author

Looks good to me @Akashsri3bi!

The part about ignore: use_build_context_synchronously is not very important but should be fixed one day, and that's easy with flutter 3.7 (#1815).

Thanks @monsieurtanuki , infact context.mounted will be updated in flutter 3.7

@Akashsri3bi Akashsri3bi deleted the ak2 branch February 9, 2023 19:37
@monsieurtanuki monsieurtanuki mentioned this pull request Feb 20, 2023
Closed
@monsieurtanuki monsieurtanuki mentioned this pull request Mar 12, 2023
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@monsieurtanuki monsieurtanuki monsieurtanuki approved these changes

Assignees

@Akashsri3bi Akashsri3bi

Labels
🥫 Product page
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Akashsri3bi @monsieurtanuki @codecov-commenter