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

fix: The nutrition page is unusable on iOS #4265

Merged
merged 1 commit into from
Jul 16, 2023

Conversation

g123k
Copy link
Collaborator

@g123k g123k commented Jul 2, 2023

Hi everyone,

On iOS, the nutrition page is pretty much unusable because we create focus nodes on every builds.
This is absolutely not necessary and creates a weird issue, preventing sometimes entering values.

Before: https://github.com/openfoodfacts/smooth-app/assets/246838/e1672ad1-7d24-41aa-9232-81e75b8168f0
After: https://github.com/openfoodfacts/smooth-app/assets/246838/be172123-46f9-4c2d-b296-79271d01c173

@g123k g123k added 🍎 iOS iOS specific issues or PRs nutrition facts labels Jul 2, 2023
@g123k g123k requested a review from a team as a code owner July 2, 2023 14:01
@g123k g123k self-assigned this Jul 2, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jul 2, 2023

Codecov Report

Merging #4265 (9c3ede2) into develop (7487b72) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           develop    #4265      +/-   ##
===========================================
- Coverage    10.81%   10.81%   -0.01%     
===========================================
  Files          287      287              
  Lines        14216    14221       +5     
===========================================
  Hits          1538     1538              
- Misses       12678    12683       +5     
Impacted Files Coverage Δ
...h_app/lib/pages/product/nutrition_page_loaded.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.

Hi @g123k!

As we're sometimes going back and forth on the matter of focus, especially when the behavior is different on iOS and android, let me suggest two things:

  1. what would happen if we didn't use explicitly focus and let flutter figure out that widgets in a column should be dealt with in the same order? Would that work? Wouldn't the code be easier to maintain then?
  2. if the previous suggestion fails, I strongly suggest an explicit comment like "CAUTION - LEAVE THE FOCUS AS THEY ARE - DIFFERENT BEHAVIOR ON IOS AND ANDROID"

@g123k
Copy link
Collaborator Author

g123k commented Jul 3, 2023

Hi @g123k!

As we're sometimes going back and forth on the matter of focus, especially when the behavior is different on iOS and android, let me suggest two things:

  1. what would happen if we didn't use explicitly focus and let flutter figure out that widgets in a column should be dealt with in the same order? Would that work? Wouldn't the code be easier to maintain then?
  2. if the previous suggestion fails, I strongly suggest an explicit comment like "CAUTION - LEAVE THE FOCUS AS THEY ARE - DIFFERENT BEHAVIOR ON IOS AND ANDROID"

I think the issue was actually on both platforms and not only on iOS.
But keyboards are managed differently, hence the fact that's more visible on iOS.

Depending on the default behavior could work, but for accessibility, it's better to explicitly provide the next node.
So, I would let the code as-is, because the only issue we had, was the fact that FocusNodes should remain between builds.

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.

@g123k Ok then!
Was probably caused by a recent PR like #4187 (2 weeks ago) because I used the nutrition screen last month for my tests on android and didn't notice anything strange.
Thank you for the fix!

@g123k g123k force-pushed the focus_nodes_nutrition_page branch from 093c77a to 9c3ede2 Compare July 16, 2023 17:13
@g123k
Copy link
Collaborator Author

g123k commented Jul 16, 2023

I forgot to merge this one, so let's do this.
And thanks @monsieurtanuki for your review!

@g123k g123k merged commit faf9863 into openfoodfacts:develop Jul 16, 2023
@g123k g123k deleted the focus_nodes_nutrition_page branch July 16, 2023 17:24
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.

3 participants