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: Product not found dialog - a fully responsive implementation #4315

Merged
merged 1 commit into from
Aug 3, 2023

Conversation

g123k
Copy link
Collaborator

@g123k g123k commented Jul 15, 2023

Hi everyone,

In Dialogs, using LayoutBuilder, AutoSizeText and in some cases Expanded, can break on some devices.
Most of the time, it's OK on real devices, but not on emulators (don't ask me why, that's what I've found on the Flutter github 🤷‍♂️).

I've rewritten the dialog to remain responsive without a LayoutBuilder.
The drawback is that it requires some manual computation.

As always, a video to show how responsive it is:
https://github.com/openfoodfacts/smooth-app/assets/246838/b918e942-42e9-4840-90f8-9057c36565c6

It will fix #4313

@g123k g123k requested a review from a team as a code owner July 15, 2023 08:06
@g123k g123k self-assigned this Jul 15, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jul 15, 2023

Codecov Report

Merging #4315 (5c09a05) into develop (899cea6) will increase coverage by 0.01%.
The diff coverage is 13.33%.

@@             Coverage Diff             @@
##           develop    #4315      +/-   ##
===========================================
+ Coverage    10.45%   10.47%   +0.01%     
===========================================
  Files          291      291              
  Lines        14416    14431      +15     
===========================================
+ Hits          1507     1511       +4     
- Misses       12909    12920      +11     
Impacted Files Coverage Δ
...app/lib/generic_lib/widgets/smooth_responsive.dart 15.38% <0.00%> (-2.27%) ⬇️
...ib/pages/product/common/product_dialog_helper.dart 0.00% <0.00%> (ø)
...p/lib/generic_lib/dialogs/smooth_alert_dialog.dart 76.12% <100.00%> (+0.63%) ⬆️

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

@github-actions
Copy link
Contributor

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 I do believe we would be better off

  • with the previous code
  • without the LayoutBuilder
  • with the svg width computed as MediaQuery.of(context).size.width / 4, as it's the only place where we use the layout constraints

Therefore, less complexity in the code.

@g123k
Copy link
Collaborator Author

g123k commented Jul 15, 2023

@g123k I do believe we would be better off

  • with the previous code
  • without the LayoutBuilder
  • with the svg width computed as MediaQuery.of(context).size.width / 4, as it's the only place where we use the layout constraints

Therefore, less complexity in the code.

Actually, it's basically your code without a LayoutBuilder.
The only difference is to add some extra paddings between elements, because clearly, outside small devices, it's too dense

@monsieurtanuki
Copy link
Contributor

The only difference is to add some extra paddings between elements, because clearly, outside small devices, it's too dense

That's what I don't get: I suppose that a Column with maxSize and spaceBetween would take all the available space with identical spaces between. Therefore all the available space. And spaces between items.

@g123k
Copy link
Collaborator Author

g123k commented Jul 16, 2023

The only difference is to add some extra paddings between elements, because clearly, outside small devices, it's too dense

That's what I don't get: I suppose that a Column with maxSize and spaceBetween would take all the available space with identical spaces between. Therefore all the available space. And spaces between items.

The main difference is that our implementation works, but depends on lots of Expanded (which is OK).
However Expanded also means that the text can be vertically cropped if there is not enough space, hence the need of AutosizeText.

The idea is my change prevents the use of AutosizeText, with more or less the same rendering at the end.

@teolemon teolemon merged commit 7f3bddf into openfoodfacts:develop Aug 3, 2023
7 checks passed
@teolemon
Copy link
Member

teolemon commented Aug 3, 2023

let's improve on that later, the modal is at least displayed now.

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.

Search for an unknown product: "LayoutBuilder does not support returning intrinsic dimensions."
4 participants