feat: Horizontal buttons for Dialogs #2626

g123k · 2022-07-18T23:26:52Z

Fix issues introduced with #2587, but also:

Supports smaller screens by reducing the margins/paddings and uses by default a vertical axis for this kind of device
Add Tooltips for the contributors' grid (will improve accessibility)
Improved scrollbars / ClipRect
Fix an overflow for the empty user lists' screen

Will fix #2622

…dfacts#2587)" (openfoodfacts#2608)" This reverts commit 6b52473.

g123k · 2022-07-19T12:11:13Z

Implementing golden tests takes longer than expected, especially because I have to mock both SQLite and Hive.
Will Hive soon be removed?

monsieurtanuki · 2022-07-19T12:22:17Z

I've never heard that hive should be removed, but why not.
It would be even safer to keep hive instead of SharedPreferences.

g123k · 2022-07-20T03:35:52Z

Okay, I thought the integration of SQLite was to migrate everything.
In that case, I will split my PR : one for the dialogs and one for the tests.
Otherwise it will be a « bloubiboulga »

monsieurtanuki · 2022-07-20T05:42:20Z

Okay, I thought the integration of SQLite was to migrate everything.

The main problem was with Products, that take a lot of space and memory.
No rush to migrate lists too, especially when there's the question "should we handle lists as SQL lists (one row per item) or with JSON (one row per list)".
Anyway, we won't migrate before the current PR is merged :)

In that case, I will split my PR : one for the dialogs and one for the tests.

Good.

Otherwise it will be a « bloubiboulga »

I've double-checked on Stackoverflow: there's a typo here, the correct name is Gloubi-boulga. Can only be found in France. Still waiting for its Nova score, help needed! (could be an easter egg)

g123k · 2022-07-20T22:20:10Z

Actually everything was ready for this part of the PR.
So moving it to a non-draft/"published" state

codecov-commenter · 2022-07-20T23:02:14Z

Codecov Report

Merging #2626 (384adcc) into develop (2ea0da3) will decrease coverage by 1.58%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           develop   #2626      +/-   ##
==========================================
- Coverage     8.86%   7.27%   -1.59%     
==========================================
  Files          161     220      +59     
  Lines         6623   10680    +4057     
==========================================
+ Hits           587     777     +190     
- Misses        6036    9903    +3867

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.70% <0.00%> (-18.51%)`	⬇️
...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 238 more

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

monsieurtanuki

Hi @g123k!
Looks OK, though I have several comments.
But my major concern is about screenshots: please take the time to share with us screenshots of the dialogs that provoked #2605 (in both small device and "not small" device mode).

monsieurtanuki · 2022-08-02T08:44:35Z

packages/smooth_app/lib/pages/preferences/user_preferences_contribute.dart

@@ -280,8 +284,10 @@ class UserPreferencesContribute extends AbstractUserPreferences {
              onPressed: () => LaunchUrlHelper.launchURL(
                  'https://github.com/openfoodfacts/smooth-app', false),
              text: AppLocalizations.of(context).contribute,
-              minWidth: 200,
+              minWidth: 150,


A bit from nowhere. Something related to screenSize would be more responsive.

monsieurtanuki · 2022-08-02T08:46:25Z

packages/smooth_app/lib/pages/preferences/user_preferences_contribute.dart

-                                contributorsData as Map<String, dynamic>);
-                        return Padding(
-                          padding: const EdgeInsets.all(5.0),
+                  return Wrap(


No scrolling needed here?

monsieurtanuki · 2022-08-02T08:52:59Z

packages/smooth_app/lib/generic_lib/widgets/smooth_responsive.dart

+  bool isTabletDevice() {
+    return size.width <= _MAX_TABLET_WIDTH;
+  }
+
+  bool isLargeDevice() {


Names are a bit confusing: I would assume a tablet to be larger than a large (allegedly smartphone) device.
What about the Android style, something like x, xx, xxx?

monsieurtanuki · 2022-08-02T08:56:04Z

packages/smooth_app/lib/generic_lib/widgets/smooth_responsive.dart

+  }
+}
+
+extension MediaQueryResponsiveExtensions on MediaQueryData {


I'll never get used to that syntax :)

Besides, in that case I guess SmoothResponsive would be good enough; the added value of the extension doesn't look interesting.

monsieurtanuki · 2022-08-02T08:58:31Z

packages/smooth_app/lib/generic_lib/widgets/smooth_responsive.dart

+
+/// Custom Widget to provide a responsive behavior.
+/// [defaultDeviceBuilder] is mandatory and will be used if no value is provided
+class SmoothResponsiveBuilder extends StatelessWidget {


Maybe I'm wrong but I haven't seen a single piece of code using it. If you don't use it now, please get rid of it: it adds complexity. Same remark for BuildContextResponsiveExtensions.

monsieurtanuki · 2022-08-02T08:59:00Z

packages/smooth_app/lib/generic_lib/widgets/smooth_responsive.dart

@@ -0,0 +1,91 @@
+import 'package:flutter/widgets.dart';
+
+class SmoothResponsive {


Some /// comments wouldn't hurt.

monsieurtanuki · 2022-08-02T09:00:00Z

packages/smooth_app/lib/generic_lib/dialogs/smooth_alert_dialog.dart

@@ -219,6 +366,7 @@ class _SmoothActionElevatedButton extends StatelessWidget {
    final ThemeData themeData = Theme.of(context);
    return SmoothSimpleButton(
      onPressed: buttonData.onPressed,
+      minWidth: buttonData.minWidth ?? 20.0,


There should probably be MINIMUM_TOUCH_SIZE at least.

monsieurtanuki · 2022-08-02T09:02:45Z

packages/smooth_app/lib/generic_lib/dialogs/smooth_alert_dialog.dart


    return AlertDialog(
-      scrollable: true,
+      scrollable: false,


Unless you're not lucky.

monsieurtanuki · 2022-08-02T09:05:15Z

packages/smooth_app/lib/generic_lib/dialogs/smooth_alert_dialog.dart

  static const EdgeInsets _contentPadding = EdgeInsets.only(
-    left: 24.0,
+    left: 22.0,
    top: VERY_LARGE_SPACE,
-    right: 24.0,
-    bottom: 24.0,
+    right: 22.0,
+    bottom: 22.0,
  );


What about EdgeInsets.all(VERY_LARGE_SPACE)?

teolemon · 2022-08-09T16:54:33Z

Ok, @g123k can I merge this one ?
typically #2758 is an example where this PR is going to improve things

g123k · 2022-08-09T18:21:42Z

Ok, @g123k can I merge this one ? typically #2758 is an example where this PR is going to improve things

Let me first change @monsieurtanuki's suggestions.
(Sorry didn't see the notification about this PR before)

teolemon · 2022-08-11T16:18:35Z

ok, really keen on merging this one, tbh, the miniature text is an eyesore

teolemon · 2022-08-16T11:23:06Z

ok @g123k @monsieurtanuki let's merge this one and create an issue for the small improvement ?

teolemon · 2022-08-18T17:27:29Z

I'm merging, we'll do the explaning in the next PR that will actually change the dialogs. We'll have to create an issue listing all those places

g123k · 2022-08-18T17:29:31Z

The next step is to list all dialogs where we want this feature.
Please comment #2805

g123k added 2 commits July 17, 2022 16:45

Revert "Revert "feat: Smooth Dialog with an axis for buttons (openfoo…

c1f2f75

…dfacts#2587)" (openfoodfacts#2608)" This reverts commit 6b52473.

Many fixes

25c2e2a

g123k self-assigned this Jul 18, 2022

Small doc + Title & Cross in separated widgets

2342434

g123k marked this pull request as ready for review July 20, 2022 22:20

g123k requested a review from a team as a code owner July 20, 2022 22:20

Increase a little bit the line height to improve readability

db00ee7

teolemon added the dialogs label Jul 27, 2022

VaiTon approved these changes Aug 1, 2022

View reviewed changes

VaiTon requested a review from monsieurtanuki August 1, 2022 20:11

Merge branch 'develop' into horizontal_buttons_2

384adcc

monsieurtanuki reviewed Aug 2, 2022

View reviewed changes

teolemon added the P2 label Aug 16, 2022

teolemon merged commit ddb8bea into openfoodfacts:develop Aug 18, 2022

github-actions bot mentioned this pull request Aug 18, 2022

chore(develop): release 3.7.0 #2735

Merged

g123k mentioned this pull request Aug 18, 2022

List all dialogs where we want vertical buttons #2805

Closed

5 tasks

g123k deleted the horizontal_buttons_2 branch September 2, 2022 06:03

github-actions bot mentioned this pull request Sep 6, 2022

chore(develop): release 3.14.0 #2951

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Horizontal buttons for Dialogs #2626

feat: Horizontal buttons for Dialogs #2626

g123k commented Jul 18, 2022 •

edited

Loading

g123k commented Jul 19, 2022

monsieurtanuki commented Jul 19, 2022

g123k commented Jul 20, 2022

monsieurtanuki commented Jul 20, 2022

g123k commented Jul 20, 2022

codecov-commenter commented Jul 20, 2022 •

edited

Loading

monsieurtanuki left a comment

monsieurtanuki Aug 2, 2022

monsieurtanuki Aug 2, 2022

monsieurtanuki Aug 2, 2022

monsieurtanuki Aug 2, 2022

monsieurtanuki Aug 2, 2022

monsieurtanuki Aug 2, 2022

monsieurtanuki Aug 2, 2022

monsieurtanuki Aug 2, 2022

monsieurtanuki Aug 2, 2022

teolemon commented Aug 9, 2022

g123k commented Aug 9, 2022

teolemon commented Aug 11, 2022

teolemon commented Aug 16, 2022

teolemon commented Aug 18, 2022

g123k commented Aug 18, 2022

		@@ -0,0 +1,91 @@
		import 'package:flutter/widgets.dart';

		class SmoothResponsive {

feat: Horizontal buttons for Dialogs #2626

feat: Horizontal buttons for Dialogs #2626

Conversation

g123k commented Jul 18, 2022 • edited Loading

g123k commented Jul 19, 2022

monsieurtanuki commented Jul 19, 2022

g123k commented Jul 20, 2022

monsieurtanuki commented Jul 20, 2022

g123k commented Jul 20, 2022

codecov-commenter commented Jul 20, 2022 • edited Loading

Codecov Report

monsieurtanuki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teolemon commented Aug 9, 2022

g123k commented Aug 9, 2022

teolemon commented Aug 11, 2022

teolemon commented Aug 16, 2022

teolemon commented Aug 18, 2022

g123k commented Aug 18, 2022

g123k commented Jul 18, 2022 •

edited

Loading

codecov-commenter commented Jul 20, 2022 •

edited

Loading