-
-
Notifications
You must be signed in to change notification settings - Fork 285
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: added dialog if users click on unselect image button #2427 #3659
Conversation
Openfoodfacts_screenrec.mov@monsieurtanuki @teolemon Please review, here's the screen recording and please let me know if there are any changes to be made. |
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 @BhuvanAde!
Please have a look at my comments.
And even more important: use branches for PRs, not develop
.
packages/smooth_app/lib/pages/product/product_image_viewer.dart
Outdated
Show resolved
Hide resolved
I'm really sorry @monsieurtanuki , I have made the suggested changes. I'm really sorry I will make sure to create a new branch next time. Please let me know if there are any other changes required. |
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.
Still can't see a good reason to have any change in pubspec.lock
. Please remove it from the PR.
@monsieurtanuki I have changed the |
I think there are conflicts. Shall I send the PR again? Please let me know |
Shall I rebase and accept my correct changes? |
If you are able to do so, sure. If there are too many, you can reopen as well |
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 @BhuvanAde!
Please have a look at my comments and get rid of pubspec.lock
.
_barcode, | ||
imageField: widget.imageField, | ||
widget: this, | ||
final bool? includeLogs = await showDialog<bool>( |
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.
Very peculiar variable choice!
What about confirmed
?
_localDatabase.notifyListeners(); | ||
navigatorState.pop(); |
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.
Please put those lines back.
@@ -1624,6 +1624,12 @@ | |||
"choose_image_source_body": "Please choose a image source", | |||
"@choose_image_source_body": { | |||
"description": "Body for the image source chooser" | |||
|
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.
navigatorState.pop(); | ||
}, | ||
), | ||
if (includeLogs ?? false) { |
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.
if (includeLogs ?? false) { | |
if (includeLogs == true) { |
would be easier to read.
I will make a new PR @monsieurtanuki. |
Signed-off-by: BhuvanAde [email protected]
What
app_en.ar
Screenshot
Fixes bug(s)
#2427
Part of