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: Clicking outside the suggestion popup should close it #3754

Merged
merged 2 commits into from
Mar 2, 2023

Conversation

thomas-algo
Copy link
Collaborator

What

  • Created a widget that allows to unfocus the text field (and dismiss the keyboard) when user tap outside the text field and inside this widget.
  • Used this widget in the edit page: for the 'Packaging components' page and all the pages generated with 'simple_input_page.dart' (Categories, Labels & Cert, Stores, Origins, Traceability codes, Country and the MultipleListTile)
  • The same method doesn't seem to work with the 'Nutrition facts' page

Videos

Fixes bug(s)

Used on edit_new_packagings.dart and simple_input_page.dart
@thomas-algo thomas-algo requested a review from a team as a code owner March 2, 2023 00:41
@github-actions github-actions bot added ✏️ Editing - Packaging input Related to the structured input of food packaging. 🥫 Product page labels Mar 2, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #3754 (ac89b61) into develop (90a9bc5) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           develop    #3754      +/-   ##
===========================================
- Coverage    10.40%   10.40%   -0.01%     
===========================================
  Files          272      272              
  Lines        13835    13846      +11     
===========================================
  Hits          1440     1440              
- Misses       12395    12406      +11     
Impacted Files Coverage Δ
...oth_app/lib/pages/product/edit_new_packagings.dart 0.00% <0.00%> (ø)
...mooth_app/lib/pages/product/simple_input_page.dart 0.00% <0.00%> (ø)
...app/lib/pages/product/simple_input_text_field.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 @thomas-algo!

I'm not able to actually test your PR, and I'm worried about something: doesn't your Unfocus widget capture all gestures, preventing other widgets below to capture taps?
For instance, I'm in an autocomplete text field: how can I click on the android app bar back button if the gesture is caught by Unfocus?
That's a true question - actually I'm not very familiar with GestureDetector.

I'm a bit surprised that you passed the "format" tests, as the following doesn't look great (@M123-dev, any opinion on that?)

      child: UnfocusWhenTapOutside(
          child: SmoothScaffold(
        appBar: SmoothAppBar(

Please don't be shy with ,, e.g. ),), instead of )),.

@thomas-algo
Copy link
Collaborator Author

Hi !
According to the doc: Given a parent GestureDetector with an onTap callback, and a child GestureDetector that also defines an onTap callback, if the child is tapped, then only the child onTap is called.
Therefore, by putting UnfocusWhenTapOutside above the Scaffold, it will not interfere with other GestureDetector. Upon testing, I did not find any problem.
About formatting, I just updated it.

@monsieurtanuki
Copy link
Contributor

I've just noticed that @Akashsri3bi was assigned this issue, therefore it's a bit problematic to give you control over this issue, unless you've already discussed the matter with @Akashsri3bi.

Now, about your latest comment and commit.

About formatting, I just updated it.

Thank you!

According to the doc: Given a parent GestureDetector with an onTap callback, and a child GestureDetector that also defines an onTap callback, if the child is tapped, then only the child onTap is called.

That would work with GestureDetectors, but I was more thinking about a GestureDetector on top of a widget that does not use a GestureDetector, like a button.

The same method doesn't seem to work with the 'Nutrition facts' page

In that page we don't use autocompletes anyway, do we?

@thomas-algo
Copy link
Collaborator Author

thomas-algo commented Mar 2, 2023

I've just noticed that @Akashsri3bi was assigned this issue, therefore it's a bit problematic to give you control over this issue, unless you've already discussed the matter with @Akashsri3bi.

I have contacted him on slack and I am waiting for a response.

That would work with GestureDetectors, but I was more thinking about a GestureDetector on top of a widget that does not use a GestureDetector, like a button.

Upon testing, it seems that the same principle applies to buttons and other: the child always has the priority.

In that page we don't use autocompletes anyway, do we?

UnfocusWhenTapOutside allows to dismiss the suggestions and the keyboard.
For 'Nutrition facts', there is no suggestions so it is less of a problem, the keyboard can be dismissed on android using the back button but not on ios according to #3283.
It might be interesting to give a solution for ios users to dismiss the keyboard, but it is not as important because the keyboard doesn't obstruct the page 'Nutrition facts'.

@Akashsri3bi
Copy link
Contributor

@monsieurtanuki Please assign it to @thomas-algo I think his approach might work for the same!

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 @thomas-algo!
I guess my similar but different experience with a GestureDetector with stronger priority involved a Stack, and not a child.

@monsieurtanuki monsieurtanuki merged commit 2b0fe2b into openfoodfacts:develop Mar 2, 2023
@monsieurtanuki
Copy link
Contributor

Thank you @Akashsri3bi for your fast answer about letting @thomas-algo deal with that issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✏️ Editing - Packaging input Related to the structured input of food packaging. 🥫 Product page
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clicking outside the suggestion popup should close it
4 participants