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: AutocompleteWidget: Scrollbar + dividers + correct width #2704

Merged
merged 2 commits into from
Aug 1, 2022

Conversation

g123k
Copy link
Collaborator

@g123k g123k commented Aug 1, 2022

On product edition pages, when there are fields with suggestions, the list can be improved by:

  • Adding some dividers
  • Adding a scrollbar
  • Ensuring the content fits the screen width

Previous UI:
Screenshot_1659347776

New UI:
Screenshot_1659349478

Will fix #2703

@g123k g123k self-assigned this Aug 1, 2022
@g123k g123k requested a review from a team as a code owner August 1, 2022 10:26
@g123k g123k changed the title AutocompleteWidget: Scrollbar + dividers + correct width fix: AutocompleteWidget: Scrollbar + dividers + correct width Aug 1, 2022
@g123k
Copy link
Collaborator Author

g123k commented Aug 1, 2022

Note: Despite my PR, we should also add that clicking outside the list should close it

@codecov-commenter
Copy link

codecov-commenter commented Aug 1, 2022

Codecov Report

Merging #2704 (e445a30) into develop (2ea0da3) will decrease coverage by 1.66%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           develop   #2704      +/-   ##
==========================================
- Coverage     8.86%   7.19%   -1.67%     
==========================================
  Files          161     219      +58     
  Lines         6623   10605    +3982     
==========================================
+ Hits           587     763     +176     
- Misses        6036    9842    +3806     
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 60.00% <0.00%> (-22.98%) ⬇️
...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%) ⬇️
...p/lib/pages/onboarding/consent_analytics_page.dart 0.00% <0.00%> (-1.57%) ⬇️
...ackages/smooth_app/lib/pages/scan/scan_header.dart 3.33% <0.00%> (-1.43%) ⬇️
... and 236 more

Help us with your feedback. Take ten seconds to tell us how you rate us.

@monsieurtanuki
Copy link
Contributor

Note: Despite my PR, we should also add that clicking outside the list should close it

In a perfect world the list should probably take as much space as possible, therefore the notion of "outside the list" is not easy.
Similar suggestion: the text field "clear" button should clear the field AND collapse the list. (not sure if an empty text field triggers the auto complete or if we wait for at least 1 input character).

@VaiTon VaiTon requested a review from monsieurtanuki August 1, 2022 13:06
borderSide: BorderSide.none,
LayoutBuilder(
builder: (_, BoxConstraints constraints) {
return Row(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find reading this a bit difficult. What about moving part of it to a fun?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find reading this a bit difficult. What about moving part of it to a fun?

@VaiTon In flutter we call that Widgets ;)
That said, I must say that code is not fun to read as there are almost no changes and the only difference is that the LayoutBuilder shifts the whole code. As we use flutter and automatic format there's not much we can do about it.
Perhaps this time move that Row into an ad-hoc Widget: that won't solve @VaiTon's problem now but at least next time any change would be easier to review. No a strong requirement from my side, though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have split the child of the AutocompleteList to a dedicated Widget.
However for the simple_input_widget file, there is too much work to do, and is thus outside the goal of this PR.

Please open an issue if necessary instead 👍

@g123k g123k force-pushed the product_edition_autocomplete branch from cb41a6c to e445a30 Compare August 1, 2022 18:32
@VaiTon VaiTon merged commit 1618781 into openfoodfacts:develop Aug 1, 2022
@g123k g123k deleted the product_edition_autocomplete branch December 30, 2024 10:53
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.

Autocomplete with some UI issues
4 participants