feat: #798 - now a unique page for preferences, in accordion #820

monsieurtanuki · 2022-01-02T12:13:07Z

Deleted file:

settings_page.dart: all the code was moved to user_preferences_page.dart

Impacted files:

labeler.yml: removed reference to settings_page.dart
user_preferences.dart: added bool flags getter and setter
user_preferences_page.dart: now displays in accordion the same preferences, plus the former settings_page, plus the former action bar

It's ugly (in the code and on the screen), but it's a start

…dion Deleted file: * `settings_page.dart`: all the code was moved to `user_preferences_page.dart` Impacted files: * `labeler.yml`: removed reference to `settings_page.dart` * `user_preferences.dart`: added bool flags getter and setter * `user_preferences_page.dart`: now displays in accordion the same preferences, plus the former settings_page, plus the former action bar

teolemon · 2022-01-02T14:32:10Z

That's great @monsieurtanuki 👍
Eventually, the mocks suggest that food preferences subsections can collapse (ingredients, environment, nutrition), which echoes concerns about potentially excessive scrolling cc @M123-dev

M123-dev

First of all (again?) happy new year @monsieurtanuki and @teolemon 🎉

And to the PR there are a few things that I have to put out, see the comments but in general I think the code is much more complex than it needs to be, as can be seen by the fact that even you made a mistake (in profile are the settings items and in vrac are the user things).

ListView(
    children: <Widget>[
      
      _getGroupTitle(0, userPreferences, themeData);
      if(!_isCollapsed(0, userPreferences)) ..._getListItemsUser(...);
      
      _getGroupTitle(1, userPreferences, themeData);
      if(!_isCollapsed(1, userPreferences)) ..._getListItemsUser(...);
      
      _getGroupTitle(2, userPreferences, themeData);
      if(!_isCollapsed(2, userPreferences)) ..._getListItemsUser(...);
      
    ],
  );
  
  

  List<Widget> _getListItemsUser(){  
    return <Widget>[
      
      
      
      
    ];
};

M123-dev · 2022-01-02T14:59:37Z

packages/smooth_app/lib/pages/user_preferences_page.dart

+    throw Exception('unknown group index: $groupIndex');
+  }
+
+  int _getTotalSize(


Suggestion: _getTotalLength

M123-dev · 2022-01-02T15:00:36Z

packages/smooth_app/lib/pages/user_preferences_page.dart

+          _getTotalSize(groups, userPreferences),
+          (int index) {
+            for (int groupIndex = 0; groupIndex < 3; groupIndex++) {
+              if (index-- == 0) {


This syntax works but it's not so easy to understand, I would prefer something like this but feel free to ignore that

index--; if(index == 0){

There are indeed things to fix in my code.
But here your suggestion is wrong:

if(index-- == 0){ // do something with index before subtracting 1 }

means

if(index == 0){ index --; // do something } else { index--; }

M123-dev · 2022-01-02T15:05:38Z

packages/smooth_app/lib/pages/user_preferences_page.dart

+    if (extra) {
+      title = Row(
+        mainAxisAlignment: MainAxisAlignment.start,
+        crossAxisAlignment: CrossAxisAlignment.start,
+        children: [
+          title,
+          Container(
+            width: 15,
+            height: 15,
+            child: Center(
+              child: Text(
+                '1',
+                style: TextStyle(
+                  color: Colors.white,
+                  fontSize: 8,
+                  fontWeight: FontWeight.bold,
+                ),
+              ),
+            ),
+            decoration: BoxDecoration(
+              color: Colors.red,
+              shape: BoxShape.circle,
+            ),
+          ),
+        ],
+      );


I don't think we need this red dot from the mocks, it does look good, but only a hardcoded one is confusing. We currently have nothing to display with this so we don't need the additional code.

Fair enough, it was just for the screenshot. Removing it...

M123-dev · 2022-01-02T15:10:46Z

packages/smooth_app/lib/pages/user_preferences_page.dart

+        return 5;
+      case 1:
+        return groups.length;
+      case 2:
+        return 3;


A user items length and settings items length int would certainly be nice for later additions

M123-dev · 2022-01-02T15:11:08Z

packages/smooth_app/lib/pages/user_preferences_page.dart

+    );
+  }
+
+  int _getCollapsedSize(


_getExpandedLength

M123-dev · 2022-01-02T15:28:00Z

packages/smooth_app/lib/pages/user_preferences_page.dart

+      case 1:
+        return 'food';
+      case 2:
+        return 'vrac';


Had to use the translator to understand this, would be good to stick with english

M123-dev · 2022-01-02T15:29:31Z

packages/smooth_app/lib/pages/user_preferences_page.dart

+  ) {
+    switch (groupIndex) {
+      case 0:
+        return _getListItemSettings(


_getListItemVrac

M123-dev · 2022-01-02T15:29:40Z

packages/smooth_app/lib/pages/user_preferences_page.dart

+          themeData,
+        );
+      case 2:
+        return _getListItemVrac(index, appLocalizations);


_getListItemSettings

New files: * `abstract_user_preferences.dart`: Abstraction of a collapsed/expanded display for the preferences page. * `user_preferences_attribute_group.dart`: Collapsed/expanded display of attributes for the preferences page. * `user_preferences_profile.dart`: Collapsed/expanded display of profile for the preferences page. * `user_preferences_settings.dart`: Collapsed/expanded display of settings for the preferences page. Impacted file: * `user_preferences_page.dart`: huge simplification through the use of instances of new class `AbstractUserPreferences`

monsieurtanuki · 2022-01-02T18:41:56Z

Thank you guys for your code review!
@M123-dev I've just cleaned the code "à la Java".
@teolemon I think I could also implement collapsed/expanded attribute groups: that shouldn't be a problem.

teolemon · 2022-01-02T21:08:49Z

@monsieurtanuki there are conflicts to solve

…e.dart

…e.dart part 2/2 so far so good

…feature/openfoodfacts#798

monsieurtanuki · 2022-01-03T07:07:54Z

Conflict resolved.

teolemon · 2022-01-03T08:30:45Z

info • The import of 'package:flutter/cupertino.dart' is unnecessary because all of the used elements are also provided by the import of 'package:flutter/material.dart' • packages/smooth_app/lib/pages/user_preferences_attribute_group.dart:1:8 • unnecessary_import
info • The import of 'package:flutter/cupertino.dart' is unnecessary because all of the used elements are also provided by the import of 'package:flutter/material.dart' • packages/smooth_app/lib/pages/user_preferences_page.dart:1:8 • unnecessary_import
info • The import of 'package:flutter/cupertino.dart' is unnecessary because all of the used elements are also provided by the import of 'package:flutter/material.dart' • packages/smooth_app/lib/pages/user_preferences_profile.dart:1:8 • unnecessary_import

teolemon · 2022-01-03T08:31:30Z

packages/smooth_app/lib/pages/user_preferences_attribute_group.dart

@@ -0,0 +1,168 @@
+import 'package:flutter/cupertino.dart';


Suggested change

import 'package:flutter/cupertino.dart';

packages/smooth_app/lib/pages/user_preferences_page.dart

packages/smooth_app/lib/pages/user_preferences_profile.dart

teolemon · 2022-01-03T08:33:30Z

I added 3 suggestions to 🟢 the CI @monsieurtanuki @M123-dev

M123-dev

Looks great, thanks @monsieurtanuki, maybe it would be an idea to move the preferences pages to their own subfolder but it's not that important

M123-dev · 2022-01-03T11:55:41Z

packages/smooth_app/lib/pages/abstract_user_preferences.dart

+abstract class AbstractUserPreferences {
+  AbstractUserPreferences(this.setState);


Just a suggestion I think AppPreferences is easier to otherwise it could be mistakely seen as the real user manangement setting, but feel free to ignore that

I added 3 suggestions to 🟢 the CI @monsieurtanuki @M123-dev

I'll do that. Is there a way we can put the same level of lint checks in dev (Android Studio) and in CI?

M123-dev · 2022-01-03T12:08:26Z

packages/smooth_app/lib/pages/user_preferences_page.dart

+      UserPreferencesSettings(setState),
+      UserPreferencesAttributeGroup(
+        setState,
+        productPreferences,


Shouldn't UserPreferencesProfile be the first one here?

New file: * `user_preferences_food.dart`: Collapsed/expanded display of attribute groups for the preferences page. Impacted files: * `abstract_user_preferences.dart`: refactored for localization * `app_en.arb`: added 6 labels for preferences titles and subtitles * `app_fr.arb`: added 6 labels for preferences titles and subtitles * `user_preferences_attribute_group.dart`: now we manage here only one group at a time, to make it collapsable * `user_preferences_page.dart`: refactored for localization * `user_preferences_profile.dart`: refactored for localization * `user_preferences_settings.dart`: refactored for localization

monsieurtanuki · 2022-01-03T14:26:19Z

Now we have localization and collapsable food preferences:

Impacted file: * `user_preferences_food.dart`

Impacted files: * `user_preferences_page-blue-dark.png` * `user_preferences_page-blue-light.png` * `user_preferences_page-brown-dark.png` * `user_preferences_page-brown-light.png` * `user_preferences_page-green-dark.png` * `user_preferences_page-green-light.png`

monsieurtanuki · 2022-01-03T14:39:53Z

Btw how is "labeler" supposed to work? Every time it fails - is there something to fix, and what?

teolemon · 2022-01-03T14:44:52Z

labeler works if you PR on the main repo, but not on forks :-(
pyrsia/pyrsia#130

teolemon approved these changes Jan 2, 2022

View reviewed changes

M123-dev requested changes Jan 2, 2022

View reviewed changes

monsieurtanuki requested a review from M123-dev January 2, 2022 18:38

teolemon added food preferences ⚙️ Settings labels Jan 2, 2022

monsieurtanuki added 4 commits January 3, 2022 07:50

feat: openfoodfacts#798 - conflicts desperate fix around settings_pag…

b9a4ff1

…e.dart

Merge branch 'develop' into feature/openfoodfacts#798

5761357

feat: openfoodfacts#798 - conflicts desperate fix around settings_pag…

bdf5557

…e.dart part 2/2 so far so good

Merge remote-tracking branch 'origin/feature/openfoodfacts#798' into …

0dac0b9

…feature/openfoodfacts#798

teolemon reviewed Jan 3, 2022

View reviewed changes

packages/smooth_app/lib/pages/user_preferences_page.dart Outdated Show resolved Hide resolved

teolemon reviewed Jan 3, 2022

View reviewed changes

packages/smooth_app/lib/pages/user_preferences_profile.dart Outdated Show resolved Hide resolved

M123-dev approved these changes Jan 3, 2022

View reviewed changes

monsieurtanuki added 2 commits January 3, 2022 15:27

feat: openfoodfacts#798 - no cupertino allowed!

a6f8ee2

Impacted file: * `user_preferences_food.dart`

monsieurtanuki merged commit 5ba0310 into openfoodfacts:develop Jan 3, 2022

This was referenced Jan 3, 2022

Turn the preferences tab into an accordeon #798

Closed

Bad clipping on top of food preferences due to Settings and User management (iPhone XS) #815

Closed

github-actions bot mentioned this pull request Jan 15, 2022

chore(develop): release 1.0.0 #962

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: #798 - now a unique page for preferences, in accordion #820

feat: #798 - now a unique page for preferences, in accordion #820

monsieurtanuki commented Jan 2, 2022

teolemon commented Jan 2, 2022

M123-dev left a comment •

edited

Loading

M123-dev Jan 2, 2022

M123-dev Jan 2, 2022

monsieurtanuki Jan 2, 2022

M123-dev Jan 2, 2022

monsieurtanuki Jan 2, 2022

M123-dev Jan 2, 2022

M123-dev Jan 2, 2022

M123-dev Jan 2, 2022

M123-dev Jan 2, 2022

M123-dev Jan 2, 2022

monsieurtanuki commented Jan 2, 2022

teolemon commented Jan 2, 2022

monsieurtanuki commented Jan 3, 2022

teolemon commented Jan 3, 2022 •

edited

Loading

teolemon Jan 3, 2022

teolemon commented Jan 3, 2022

M123-dev left a comment

M123-dev Jan 3, 2022

monsieurtanuki Jan 3, 2022

M123-dev Jan 3, 2022

monsieurtanuki commented Jan 3, 2022

monsieurtanuki commented Jan 3, 2022

teolemon commented Jan 3, 2022

		abstract class AbstractUserPreferences {
		AbstractUserPreferences(this.setState);

feat: #798 - now a unique page for preferences, in accordion #820

feat: #798 - now a unique page for preferences, in accordion #820

Conversation

monsieurtanuki commented Jan 2, 2022

teolemon commented Jan 2, 2022

M123-dev left a comment • edited Loading

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

Choose a reason for hiding this comment

monsieurtanuki commented Jan 2, 2022

teolemon commented Jan 2, 2022

monsieurtanuki commented Jan 3, 2022

teolemon commented Jan 3, 2022 • edited Loading

Choose a reason for hiding this comment

teolemon commented Jan 3, 2022

M123-dev 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

monsieurtanuki commented Jan 3, 2022

monsieurtanuki commented Jan 3, 2022

teolemon commented Jan 3, 2022

M123-dev left a comment •

edited

Loading

teolemon commented Jan 3, 2022 •

edited

Loading