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: use modals for navigating to editing screens #2797

Merged

Conversation

simonbengtsson
Copy link
Contributor

What

  • Present editing screens as modals on iOS to make it natural to disable back navigation gesture

Fixes issues

Potential future improvements

Modals in iOS are always presented on top of bottom tab bars. I found a way to do this fairly easily by using the root navigator instead of the top navigator. The problem was that it started generating this warning about duplicate hero tags: There are multiple heroes that share the same tag within a subtree. I couldn't figure out why this was the case and haven't worked with Hero's or nested navigators much so left it as is.

For future reference, this change enabled modals to cover the entire screen (including bottom tab bar):

Replace

await Navigator.push<Product>(context, <route>, fullscreenDialog: true)

with

await Navigator.of(context, rootNavigator: true).push<Product>(<route>, fullscreenDialog: true)

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 @simonbengtsson!

Your code looks good, but I'm afraid there are some inconsistencies in EditProductPage: some items open a "full dialog", other items open a page (with a back button).
I guess we would be better off if all items opened the same thing, namely a "full dialog".

Regarding your remark about Navigator.push I would probably agree with you but the navigation system that was chosen was with this bottom bar "on top". Definitely not my personal choice, but that's the way it is, so let's put your remark on the shelf and use it if/when needed.

@codecov-commenter
Copy link

Codecov Report

Merging #2797 (426b0e9) into develop (2ea0da3) will decrease coverage by 1.86%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           develop   #2797      +/-   ##
==========================================
- Coverage     8.86%   6.99%   -1.87%     
==========================================
  Files          161     218      +57     
  Lines         6623   10722    +4099     
==========================================
+ Hits           587     750     +163     
- Misses        6036    9972    +3936     
Impacted Files Coverage Δ
...kages/smooth_app/lib/widgets/attribute_button.dart 0.00% <0.00%> (-92.00%) ⬇️
...s/smooth_app/lib/data_models/user_preferences.dart 8.73% <0.00%> (-23.57%) ⬇️
packages/smooth_app/lib/themes/smooth_theme.dart 62.26% <0.00%> (-20.72%) ⬇️
...p/lib/generic_lib/dialogs/smooth_alert_dialog.dart 15.11% <0.00%> (-19.10%) ⬇️
...mooth_app/lib/data_models/product_preferences.dart 21.68% <0.00%> (-9.75%) ⬇️
packages/smooth_app/lib/main.dart 14.65% <0.00%> (-3.24%) ⬇️
.../smooth_app/lib/pages/onboarding/welcome_page.dart 0.00% <0.00%> (-3.13%) ⬇️
.../smooth_app/lib/pages/onboarding/scan_example.dart 0.00% <0.00%> (-2.28%) ⬇️
...ackages/smooth_app/lib/pages/scan/scan_header.dart 2.50% <0.00%> (-2.27%) ⬇️
...p/lib/pages/onboarding/consent_analytics_page.dart 0.00% <0.00%> (-1.57%) ⬇️
... and 237 more

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

@simonbengtsson
Copy link
Contributor Author

Good point! Fixed so that all product edit related screens now opens in fullscreen in the latest commit.

@teolemon
Copy link
Member

Is that purely a technical change on how we display, or are there any visible changes ?

@teolemon teolemon added 🍎 iOS iOS specific issues or PRs ✏️ Editing Many products are incomplete and don't have Nutri-Score, Eco-Score…so editing is important for users labels Aug 17, 2022
@simonbengtsson
Copy link
Contributor Author

On Android there is no visible change, but on iOS there are two. One is that the screen animates in from the bottom instead of from the left (modal style) and the other is that the back arrow icon is changed to a cancel cross. See a screen recording of the screen presentation style here: #2796 (comment)

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 @simonbengtsson!
I'm curious to see how this works with edit screens that call other edit screens themselves, so this needs to be double-checked while using the app.

@simonbengtsson
Copy link
Contributor Author

Sounds good. Rebased against develop now.

@monsieurtanuki monsieurtanuki merged commit 3431933 into openfoodfacts:develop Aug 19, 2022
@monsieurtanuki
Copy link
Contributor

Thank you @simonbengtsson!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✏️ Editing Many products are incomplete and don't have Nutri-Score, Eco-Score…so editing is important for users 🍎 iOS iOS specific issues or PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WillPopScope prevents swipe to go back on MaterialPageRoute
4 participants