fix: Add a nudge in home for people still using org.openfoodfact.app fixes #2979 #3030

juliethangeldeveloper · 2022-09-16T22:43:16Z

What

Old deprecated app fix

added the code to check if a user is using an older version of the app and a button to update the app

Screenshot

Fixes bug(s)

https://github.com/orgs/openfoodfacts/projects/61/views/1

Fixes: People who were still using the org.openfoodfact.app

Part of

Add a big nasty button in home for people still using org.openfoodfact.app #2979

format updated

monsieurtanuki

Hi @juliethangeldeveloper!
Please have a look at my comments.

monsieurtanuki · 2022-09-17T11:34:08Z

packages/smooth_app/lib/l10n/app_en.arb

+    "depricatedHeader": "You are using a deprecated version of the app.",
+    "@depricatedHeader": {
+        "description": "Confirmation, that the user can upgrade to new version of the app"
+    },
+    "clickHereToDownload": "Click here to download",
+    "@clickHereToDownload": {
+        "description": "Confirmation button to download new version of the app"
+    },


Suggested change

"depricatedHeader": "You are using a deprecated version of the app.",

"@depricatedHeader": {

"description": "Confirmation, that the user can upgrade to new version of the app"

},

"clickHereToDownload": "Click here to download",

"@clickHereToDownload": {

"description": "Confirmation button to download new version of the app"

},

"deprecated_header": "You are using a deprecated version of the app.",

"@deprecated_header": {

"description": "Confirmation, that the user can upgrade to new version of the app"

},

"click_here_to_download": "Click here to download",

"@click_here_to_download": {

"description": "Confirmation button to download new version of the app"

},

I believe that most (all?) tags use a lower_case style.

monsieurtanuki · 2022-09-17T11:40:33Z

packages/smooth_app/lib/widgets/smooth_product_carousel.dart

+
+  /// We fetch first if the app is deprecated, then try to get the tagline
+  /// Will return [false] if the app is deprecated or a [TagLineItem]
+  Future<dynamic> _fetchData() async {


Suggested change

Future<dynamic> _fetchData() async {

Future<TagLineItem?> _fetchData() async {

dynamic is ugly; please return null if deprecated. I may be wrong but that shouldn't be a problem.

monsieurtanuki · 2022-09-17T11:41:07Z

packages/smooth_app/lib/widgets/smooth_product_carousel.dart

-                        color: Theme.of(context).colorScheme.primary,
-                      ),
-                    ),
+            return FutureBuilder<dynamic>(


Suggested change

return FutureBuilder<dynamic>(

return FutureBuilder<TagLineItem?>(

monsieurtanuki · 2022-09-17T11:43:35Z

packages/smooth_app/lib/widgets/smooth_product_carousel.dart

+            return FutureBuilder<dynamic>(
+              future: _fetchData(),
+              builder: (BuildContext context, AsyncSnapshot<dynamic> data) {
+                if (data.data != null && data.data is! TagLineItem) {


Please use instead something like data.connectionState == ConnectionState.done && data.data == null.
The is syntax should be avoided.

@g123k reviewing the comment from the code you sent me.

monsieurtanuki · 2022-09-17T11:44:38Z

packages/smooth_app/lib/widgets/smooth_product_carousel.dart

+            style: const 
+            TextStyle(
+              color: Colors.red,
+            ),)),


Suggested change

),)),

),),),

monsieurtanuki · 2022-09-17T11:45:44Z

packages/smooth_app/lib/widgets/smooth_product_carousel.dart

+          child: Text(
+            '${localizations.depricatedHeader}  ${localizations.clickHereToDownload}',


It would make more sense to have the header as a Text and then the "click!" as a TextButton.

I did try it first like this but would involve more lines of code but I agree would be better for the user. In the request of the issue, the line on the UI requested was all in one line. So I added it to the whole text as a TextButton.

Again, not sure if that looks good: please attach a screenshot.

monsieurtanuki · 2022-09-17T11:49:25Z

packages/smooth_app/lib/widgets/smooth_product_carousel.dart

+                if (data.data != null && data.data is! TagLineItem) {
+                  return const _SearchCardTagLineDeprecatedAppText();
+                } else if (data is TagLineItem) {
+                  return _SearchCardTagLineText(


No strong added value to move that code into a separate widget: please think about the poor developer that will review your code ;)

@g123k review the comment from the code you sent me

monsieurtanuki · 2022-09-17T11:50:11Z

packages/smooth_app/lib/widgets/smooth_product_carousel.dart

+  }
+
+  /// Opens the App Store or Google Play of the production app
+  Future<bool> _openAppStore() async {


Suggested change

Future<bool> _openAppStore() async {

Future<void> _openAppStore() async {

It looks like you're OK with this suggestion, so please implement it.

…or the rest of the code changes

monsieurtanuki

Hi @juliethangeldeveloper!
Still some things to fix; please have a look at my comments.

monsieurtanuki · 2022-10-02T13:55:25Z

packages/smooth_app/lib/l10n/app_en.arb

+    "@deprecated_header": {
+        "description": "Confirmation, that the user can upgrade to new version of the app"
+    },
+    "click_here_to_download": "Click here to download",


Suggested change

"click_here_to_download": "Click here to download",

"click_here_to_download": "Download the new version of the app",

monsieurtanuki · 2022-10-02T13:56:08Z

packages/smooth_app/lib/widgets/smooth_product_carousel.dart

-                      ),
-                    ),
+              future: _fetchData(),
+              builder: (BuildContext context, AsyncSnapshot<dynamic> data) {


Suggested change

builder: (BuildContext context, AsyncSnapshot<dynamic> data) {

builder: (BuildContext context, AsyncSnapshot<TagLineItem?> data) {

monsieurtanuki · 2022-10-02T13:58:10Z

packages/smooth_app/lib/widgets/smooth_product_carousel.dart

+              builder: (BuildContext context, AsyncSnapshot<dynamic> data) {
+                if (data.connectionState == ConnectionState.done && data.data == null) {
+                  return const _SearchCardTagLineDeprecatedAppText();
+                } else if (data is TagLineItem) {


Please avoid the data is TagLineItem syntax.
Here it would be data.connectionState == ConnectionState.done.

monsieurtanuki · 2022-10-02T13:59:52Z

packages/smooth_app/lib/widgets/smooth_product_carousel.dart

+          child: Text(
+            '${localizations.depricatedHeader}  ${localizations.clickHereToDownload}',


Again, not sure if that looks good: please attach a screenshot.

monsieurtanuki · 2022-10-02T14:01:28Z

packages/smooth_app/lib/widgets/smooth_product_carousel.dart

+  }
+
+  /// Opens the App Store or Google Play of the production app
+  Future<bool> _openAppStore() async {


It looks like you're OK with this suggestion, so please implement it.

monsieurtanuki

Hi @juliethangeldeveloper!
The code is improving but there are still things to fix.

Beyond my detailed remarks, let me suggest:

to find in Android Studio (or the tool you're using) how to have the code formatted (something like Ctrl-S)
to attach a screenshot

monsieurtanuki · 2022-10-02T16:48:40Z

packages/smooth_app/lib/widgets/smooth_product_carousel.dart

-              color: Colors.red,
-            ),),),
+      child: 
+      SizedBox( height: 50,


We don't need the SizedBox, do we?

yes because some devices would not alow the click here to happen

monsieurtanuki · 2022-10-02T16:49:53Z

packages/smooth_app/lib/widgets/smooth_product_carousel.dart

+              textAlign: TextAlign.center,
+              style: const 
+              TextStyle(
+                color: Colors.red,


It's very hard to read Texts in red. Just don't specify a color.

That is the color of the text in the image

monsieurtanuki · 2022-10-02T16:53:44Z

packages/smooth_app/lib/widgets/smooth_product_carousel.dart

+              style: const TextStyle(
+                color: Colors.red,
+              ),),
+              TextButton(


Again, it's hard to read red Texts.
For this specific case, I suggest a SmoothSimpleButton with red background and white text.

monsieurtanuki · 2022-10-02T16:56:55Z

packages/smooth_app/lib/l10n/app_en.arb

+    "click_here": "Click here",
+    "@click_here": {
+        "description": "Confirmation click to download new version of the app"
+    },


Suggested change

"click_here": "Click here",

"@click_here": {

"description": "Confirmation click to download new version of the app"

},

monsieurtanuki · 2022-10-02T16:58:04Z

packages/smooth_app/lib/widgets/smooth_product_carousel.dart

+                    Text(
+              localizations.download_new_version,
+              textAlign: TextAlign.center,
+              style: const TextStyle(
+                color: Colors.red,
+              ),),


Suggested change

Text(

localizations.download_new_version,

textAlign: TextAlign.center,

style: const TextStyle(

color: Colors.red,

),),

We don't need 2 texts and a button. We can get rid of the least interesting "Click here".

you asked me to add that

I asked you to put one text and one button. I suppose that a "download the new version!" label in an elevated button will be explicit enough, we don't need a "click here" label (and we don't have that much space).

monsieurtanuki · 2022-10-02T16:58:22Z

packages/smooth_app/lib/widgets/smooth_product_carousel.dart

+                  _openAppStore();
+                },
+                child: Text(
+                  localizations.click_here,


Suggested change

localizations.click_here,

localizations.download_new_version,

juliethangeldeveloper · 2022-10-02T21:41:23Z

it seems the original photo example is not what is wanted so it needs to be updated for me to make the correct changes in this PR

M123-dev

Heyy @juliethangeldeveloper the code looks good to me. We mostly don't use the as keyword here as we have everything strong typed but thats ok here.

I think what @monsieurtanuki means is to make a button out of the "Click here to download" but leave the disclaimer as a text, I agree that a button is better for usability but as this will only be seen by people using a wrong listing or a fake variant of the app thats fine.

M123-dev · 2022-10-03T16:48:18Z

In germany we have a saying which would translate to "That was a rough birth" thats applicable here. Still thanks for your effort @juliethangeldeveloper 🥳

juliethangeldeveloper · 2022-10-03T17:24:35Z

@M123-dev Just to add on to make sure I clarify most of the code was provided for me when I join the open source talks. In this code, the keyboard as was in there and I just added to display on the screen. That's why all the comments did not make sense. Thanks for closing it and resolving the issues

Deprecated app fix openfoodfacts#2979

8bd8602

juliethangeldeveloper requested a review from a team as a code owner September 16, 2022 22:43

github-actions bot assigned juliethangeldeveloper Sep 16, 2022

juliethangeldeveloper assigned alexgarel Sep 16, 2022

juliethangeldeveloper changed the title ~~Deprecated app fix #2979~~ fix deprecated app fix #2979 Sep 16, 2022

Update smooth_product_carousel.dart

bcbaec0

format updated

juliethangeldeveloper assigned g123k Sep 16, 2022

monsieurtanuki reviewed Sep 17, 2022

View reviewed changes

Update code with some of the comments. Waiting for review of @g123k f…

dad214b

…or the rest of the code changes

teolemon changed the title ~~fix deprecated app fix #2979~~ fix: Add a nudge in home for people still using org.openfoodfact.app fixes #2979 Sep 26, 2022

M123-dev mentioned this pull request Oct 1, 2022

Add a big nasty button in home for people still using org.openfoodfact.app #2979

Closed

VaiTon requested review from M123-dev and monsieurtanuki October 2, 2022 13:45

monsieurtanuki reviewed Oct 2, 2022

View reviewed changes

Update according to comments

946fdbd

M123-dev requested a review from monsieurtanuki October 2, 2022 15:26

monsieurtanuki reviewed Oct 2, 2022

View reviewed changes

M123-dev approved these changes Oct 3, 2022

View reviewed changes

M123-dev added 3 commits October 3, 2022 18:25

Merge branch 'develop' into depricatedApp

afc6049

format

6f6e313

Update smooth_product_carousel.dart

2faa763

M123-dev merged commit 74cb804 into openfoodfacts:develop Oct 3, 2022

github-actions bot mentioned this pull request Oct 3, 2022

chore(develop): release 3.21.0 #3052

Merged

	Future<dynamic> _fetchData() async {
	Future<TagLineItem?> _fetchData() async {

	return FutureBuilder<dynamic>(
	return FutureBuilder<TagLineItem?>(

		child: Text(
		'${localizations.depricatedHeader} ${localizations.clickHereToDownload}',

	Future<bool> _openAppStore() async {
	Future<void> _openAppStore() async {

	"click_here_to_download": "Click here to download",
	"click_here_to_download": "Download the new version of the app",

	builder: (BuildContext context, AsyncSnapshot<dynamic> data) {
	builder: (BuildContext context, AsyncSnapshot<TagLineItem?> data) {

	localizations.click_here,
	localizations.download_new_version,

fix: Add a nudge in home for people still using org.openfoodfact.app fixes #2979 #3030

fix: Add a nudge in home for people still using org.openfoodfact.app fixes #2979 #3030

Conversation

juliethangeldeveloper commented Sep 16, 2022

What

Screenshot

Fixes bug(s)

Part of

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

juliethangeldeveloper Sep 17, 2022 • 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

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

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

juliethangeldeveloper Oct 2, 2022 • 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

juliethangeldeveloper commented Oct 2, 2022

M123-dev left a comment • edited Loading

Choose a reason for hiding this comment

M123-dev commented Oct 3, 2022

juliethangeldeveloper commented Oct 3, 2022

juliethangeldeveloper Sep 17, 2022 •

edited

Loading

juliethangeldeveloper Oct 2, 2022 •

edited

Loading

M123-dev left a comment •

edited

Loading