-
-
Notifications
You must be signed in to change notification settings - Fork 271
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: 3332 - refactored the new crop page UI and added a camera #3402
feat: 3332 - refactored the new crop page UI and added a camera #3402
Conversation
New file: * `may_exit_page_helper.dart`: Helper class about the "You're leaving the page with unsaved changes" case. Impacted files: * `image_crop_page.dart`: made method `pickImageFile` public * `new_crop_image.dart`: added "camera" button to app bar, moved "rotate" button on app bar, moved "ok" button as FAB, added "exit without saving" dialog feature * `simple_input_page.dart`: now using new class `MayExitPageHelper`
@omkarChend1kar Would you please review it? |
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.
- There's more ambiguity because the text next to the icons has been removed.
- Ok is not clear
- The UI does not allow to go forward the mockup I have proposed some time ago in Improve the Photo Manager using new features from openfoodfacts-dart #766
@monsieurtanuki Sure, Maybe after the requested changes. |
Hi @teolemon! You're right, my suggested UI does not go towards your mockup. In your mockup
If I implement something that goes towards your mockup, it will also look a lot like the page before the crop page, that we cannot get rid of as long as we keep the old crop tool "just in case". As for do we need labels on buttons, that's probably a matter of taste. A matter of space too, especially with localizations. That said, I'm going to implement your suggested changes (aka buttons with labels), that will make it more obvious that we need to get rid of the old crop page and the additional "edit image" page. |
If all other features are off the table, here's one version that:
|
Impacted file: * `new_crop_image.dart`: moved "camera" button to bottom center, moved "rotate" button to top right, removed "ok" button
Thank you @teolemon for your suggestions! This is a screenshot from my latest commit: For the record, according to flutter it did use a decent rotate right icon (but your icon choice is definitely more explicit): |
Codecov Report
@@ Coverage Diff @@
## develop #3402 +/- ##
===========================================
+ Coverage 10.32% 10.35% +0.02%
===========================================
Files 260 259 -1
Lines 12595 12550 -45
===========================================
- Hits 1301 1300 -1
+ Misses 11294 11250 -44
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Ok now we're back to interaction issues for saving, which was handled by the "OK" button (which added an extra step). V1 autosaves new images, and subsequent crop/rotation is saved manually on a separate screen. On V1 We had a separate initial photo upload screen (with instant upload) with (client-side) cropping embedded, and a subsequent image manipulation screen with instant upload and server-side cropping/rotation… Crop/rotation happening server-side is to avoid loosing pictures, to generate training data for cropping, reduce server storage drain… We're trying to have the same screen optimize for 2 things.
At this point, and to go from first principles to this PR:
Honestly, I took 20 min writing this text, and I'm not happy with it, since it grossly misrepresents things. I'm not into writing novels for each feature/PR. Live and Collaborative fast iteration on images/mockups or a visio are worth a thousand words, and verbal iteration on what happens in each subcase, potential user reactions… |
No server change is really applied on that screen. When we exit the screen, we provide the resulting file to the caller. That's the caller that decides to upload that file (with the "confirm" button). Again, this is due to the "old crop tool" / "new custom crop tool" dilemma. If we get rid of the old crop tool, we'll have a clearer view.
Good question, I haven't checked that one. It's probably not detected by
That was more or less the purpose of the "OK" button. BUT, as said before, for historical reasons when we clicked on OK that would bring us back to the previous page with its "confirm" button.
|
@teolemon In the visual approach, you simply ignore all the potential algorithm questions and contradictions that we coders have to deal with, like "When should we save to the server?", "What if the users forget to click on the save button?" or "What if the server rejects the cropped image?". That's not rendered on figma, is it? In the specific case of this PR, my initial point was just to add a working "camera" button. It became obvious that just adding the "camera" icon would create confusion with 2 rows of 3 icons on the bottom of the screen, and I had to change the UI accordingly. My point was not to say "This is the new UI direction!", my point was "I had to change the UI, change it as you wish but don't spend too much time on it as new features/new buttons/new UX/UI will come soon". We're zig-zaging, but in the right direction. That said, there are methods like Model–view–controller in order to separate the UI from the actual code. |
Impacted files: * `crop_helper.dart`: removed the old crop tool * `image_crop_page.dart`: removed the old crop tool * `new_crop_page.dart`: added a "rotate left" button; added a "confirm" button; refactored * `Podfile.lock`: wtf * `pubspec.lock`: wtf * `pubspec.yaml`: removed the old crop tool * `rotated_crop_controller.dart`: added a "rotate left" method * `rotation.dart`: added a "rotate left" method * `user_preferences_dev_mode.dart`: removed the choice of "old crop tool"
Deleted files: * `confirm_and_upload_picture.dart` * `crop_helper.dart` Impacted files: * `image_crop_page.dart`: simplified with fewer steps * `new_crop_page.dart`: now uploads * `product_image_viewer.dart`: minor refactoring - going directly to `CropPage`
Screenshot with the latest commit.
|
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.
Thank you very much @monsieurtanuki for the changes. We're improving the photo situation, bit by bit.
Thank you @teolemon for your feedbacks! |
New file:
may_exit_page_helper.dart
: Helper class about the "You're leaving the page with unsaved changes" case.Impacted files:
image_crop_page.dart
: made methodpickImageFile
publicnew_crop_image.dart
: added "camera" button to app bar, moved "rotate" button on app bar, moved "ok" button as FAB, added "exit without saving" dialog featuresimple_input_page.dart
: now using new classMayExitPageHelper
What
SimpleInputPage
Screenshots
Part of