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: 3459 - Added a shortcut in app settings to In-app settings #3726

Merged
merged 8 commits into from
Feb 26, 2023

Conversation

Adiii1436
Copy link
Contributor

Fixed #3459
photo_2023-02-21_23-56-39

@Adiii1436 Adiii1436 requested a review from a team as a code owner February 21, 2023 18:29
@Adiii1436 Adiii1436 changed the title Added a shortcut in app settings to In-app settings fix: 3459 - Added a shortcut in app settings to In-app settings Feb 21, 2023
Widget build(BuildContext context) {
final AppLocalizations appLocalizations = AppLocalizations.of(context);

return GestureDetector(
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey, listTile gives onTap functionality, we can make use of that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ListTile only contains the text. If I'll provide ontap() function here app-settings will only open when I'll click on text area not on the setting icon. App-settings should open when I click wherever on that particular row.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, listtile gives leading and trailiing property, use that instead of seperate padding. so onTap on list tile will handle tap on anywhere on whole list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! this should be the nice solution. Thanks @Sudhanva-Nadiger let me make changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have made changes @Sudhanva-Nadiger

@@ -45,6 +45,7 @@ dependencies:
url_launcher: 6.1.3
visibility_detector: 0.3.3
assorted_layout_widgets: 6.1.1
app_settings:
Copy link
Contributor

Choose a reason for hiding this comment

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

add version of the package

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@Sudhanva-Nadiger Sudhanva-Nadiger left a comment

Choose a reason for hiding this comment

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

Lgtm !

@@ -1521,4 +1528,4 @@ packages:
version: "3.1.1"
sdks:
dart: ">=2.17.0 <3.0.0"
flutter: ">=3.0.0"
flutter: ">=3.0.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Roll this back !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@Sudhanva-Nadiger
Copy link
Contributor

@M123-dev why test is failing?

@codecov-commenter
Copy link

codecov-commenter commented Feb 23, 2023

Codecov Report

Merging #3726 (4801d3a) into develop (cad3041) will decrease coverage by 0.01%.
The diff coverage is 5.88%.

@@             Coverage Diff             @@
##           develop    #3726      +/-   ##
===========================================
- Coverage    10.42%   10.41%   -0.01%     
===========================================
  Files          272      272              
  Lines        13804    13821      +17     
===========================================
+ Hits          1439     1440       +1     
- Misses       12365    12381      +16     
Impacted Files Coverage Δ
...b/pages/preferences/user_preferences_settings.dart 8.71% <5.88%> (-0.22%) ⬇️

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

Comment on lines 371 to 375
style: const TextStyle(
color: Color.fromRGBO(193, 193, 193, 1.0),
letterSpacing: 0.5,
fontSize: 14,
),
Copy link
Member

Choose a reason for hiding this comment

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

We have a global text style

Theme.of(context).textTheme.bodyText2

(or bodyText1 or something else in there)

Could you use this one, if you need more fine control you can use .copyWith( ... ) behind it

Copy link
Member

Choose a reason for hiding this comment

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

This way it's working with dark and light theme and other factors like accessibility

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made the changes @M123-dev

Copy link
Member

@M123-dev M123-dev left a comment

Choose a reason for hiding this comment

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

Looks good, thanks @Adiii1436

@M123-dev M123-dev merged commit 9113c4a into openfoodfacts:develop Feb 26, 2023
@Adiii1436 Adiii1436 deleted the fixed#3459 branch February 26, 2023 11:21
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.

Add a shortcut in app settings to In-app settings
5 participants