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: 2987 - The section header are smaller and not as color flashy as the titles of the individual settings. #3696

Merged
merged 17 commits into from
May 19, 2023

Conversation

Adiii1436
Copy link
Contributor

@Adiii1436 Adiii1436 commented Feb 11, 2023

Fixed #2987
Increased header font size and provided bold white color.

@Adiii1436 Adiii1436 requested a review from a team as a code owner February 11, 2023 17:25
@Adiii1436 Adiii1436 changed the title fix:The section header are smaller and not as color flashy as the titles of the individual settings. fix: 2987 - The section header are smaller and not as color flashy as the titles of the individual settings. Feb 11, 2023
Copy link
Member

@AshAman999 AshAman999 left a comment

Choose a reason for hiding this comment

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

Also can you attach a screnshot , how this affects the current UI

@Adiii1436
Copy link
Contributor Author

It will look like this:

photo_2023-02-12_22-49-08

photo_2023-02-12_22-50-34

And as you said I have made changes to userPreferencesTitle and removed the repeated code.

@Adiii1436 Adiii1436 requested a review from AshAman999 February 13, 2023 13:30
@Adiii1436
Copy link
Contributor Author

@M123-dev @teolemon I have made changes as asked by @AshAman999 . Please review the changes.

style: Theme.of(context).textTheme.displaySmall?.copyWith(
height: 2.5,
),
style: const TextStyle(fontSize: 24, fontWeight: FontWeight.bold),
Copy link
Member

@M123-dev M123-dev Feb 16, 2023

Choose a reason for hiding this comment

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

Thanks a lot @Adiii1436

Could you keep the

Theme.of(context).textTheme.displaySmall?.copyWith(

and do your changed in this copyWith.

This just means that you use the global text style and just change parts of it.
Say we plan to change the text font, we can just change it in the global theme and not at every text in the app

Copy link
Contributor Author

@Adiii1436 Adiii1436 Feb 16, 2023

Choose a reason for hiding this comment

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

Thankyou @M123-dev for guiding me. I will make changes right away.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@M123-dev I have made changes as you said.

@Adiii1436 Adiii1436 requested review from M123-dev and removed request for AshAman999 February 16, 2023 20:36
@codecov-commenter
Copy link

codecov-commenter commented Feb 20, 2023

Codecov Report

Merging #3696 (5643793) into develop (939b0d9) will not change coverage.
The diff coverage is 0.00%.

@@           Coverage Diff            @@
##           develop    #3696   +/-   ##
========================================
  Coverage    10.97%   10.97%           
========================================
  Files          265      265           
  Lines        13071    13071           
========================================
  Hits          1435     1435           
  Misses       11636    11636           
Impacted Files Coverage Δ
...ib/pages/preferences/user_preferences_widgets.dart 12.90% <0.00%> (ø)

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

Comment on lines 242 to 243
? const Color(0xFF000000)
: const Color(0xFFFFFFFF)),
Copy link
Member

Choose a reason for hiding this comment

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

Could you make it Colors.black and Colors.white, thats way easier to understand :D

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.

Made the changes @M123-dev

@Adiii1436 Adiii1436 requested a review from M123-dev February 25, 2023 15:08
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.

Happy to test this out. Thanks @Adiii1436

My only concern is maybe with the coming AMOLED mode, that the fixed colors could be problematic

cc: @AshAman999 the PR is blocked on you 😄 what do you say

@M123-dev M123-dev requested a review from AshAman999 February 26, 2023 10:59
@@ -235,8 +235,7 @@ class UserPreferencesTitle extends StatelessWidget {
),
child: Text(
label,
style: Theme.of(context).textTheme.displaySmall?.copyWith(
height: 2.5,
style: Theme.of(context).textTheme.displayLarge!
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
style: Theme.of(context).textTheme.displayLarge!
style: Theme.of(context).textTheme.displayLarge

@teolemon
Copy link
Member

@AshAman999

@AshAman999
Copy link
Member

This is how it will look, same color scheme as discussed in yesterday meeting,

Light Dark Amoled
Theme image image image

ps: good to have Chat GPT help me write good markdowns real quick.

@AshAman999 AshAman999 requested a review from teolemon May 19, 2023 16:15
@teolemon
Copy link
Member

perfect @AshAman999 , thanks 👌

@teolemon teolemon merged commit b88c45f into openfoodfacts:develop May 19, 2023
@AshAman999
Copy link
Member

Thanks @Adiii1436 for your PR 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

Discussion: The section header are smaller and not as color flashy as the titles of the individual settings.
5 participants