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: color issue in light mode on the top summary bar of the scan card #1634

Merged
merged 4 commits into from
May 8, 2022
Merged

fix: color issue in light mode on the top summary bar of the scan card #1634

merged 4 commits into from
May 8, 2022

Conversation

bhattabhi013
Copy link
Contributor

What

  • fixes the color issue for compatibility in light mode.
    Now, the text color is selected on runtime and set to onBackground color.
    For dark mode: onBackground = white;
    For light mode: onBackground = black;

Screenshot

image

Fixes bug(s)

Part of

@bhattabhi013 bhattabhi013 requested a review from a team as a code owner April 23, 2022 11:04
@codecov-commenter
Copy link

Codecov Report

Merging #1634 (0c8c026) into develop (2ea0da3) will decrease coverage by 0.14%.
The diff coverage is 1.77%.

@@            Coverage Diff             @@
##           develop   #1634      +/-   ##
==========================================
- Coverage     8.86%   8.71%   -0.15%     
==========================================
  Files          161     164       +3     
  Lines         6623    6792     +169     
==========================================
+ Hits           587     592       +5     
- Misses        6036    6200     +164     
Impacted Files Coverage Δ
...rds/knowledge_panels/knowledge_panels_builder.dart 2.17% <0.00%> (ø)
...s/product_cards/smooth_product_card_not_found.dart 0.00% <0.00%> (ø)
.../smooth_app/lib/data_models/onboarding_loader.dart 0.00% <ø> (ø)
...mooth_app/lib/data_models/product_query_model.dart 1.38% <ø> (ø)
...s/smooth_app/lib/data_models/user_preferences.dart 34.42% <ø> (+2.11%) ⬆️
...s/smooth_app/lib/database/dao_string_list_map.dart 0.00% <0.00%> (ø)
...ckages/smooth_app/lib/database/local_database.dart 0.00% <0.00%> (ø)
...s/smooth_app/lib/database/paged_product_query.dart 0.00% <0.00%> (ø)
...oth_app/lib/database/robotoff_questions_query.dart 0.00% <0.00%> (ø)
...ric_lib/buttons/smooth_large_button_with_icon.dart 0.00% <0.00%> (ø)
... and 22 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e51691d...0c8c026. Read the comment docs.

@teolemon teolemon linked an issue Apr 23, 2022 that may be closed by this pull request
1 task
@teolemon teolemon added this to the V1 milestone Apr 23, 2022
@M123-dev
Copy link
Member

Can you add a screenshot of unknown compatibility.

Setting all your preferences to the highest should do the job

@bhattabhi013
Copy link
Contributor Author

Hi @M123-dev,
I need some help in order to generate a screenshot of unknown compatibility.
I tried setting all my preferences to 'mandatory' and scanned some products but I'm unable to replicate the unknown compatibility test case.
I also tried with 'important' preferences but the result was still the same.
Can you help me with preferences settings and a product on which I can replicate the unknown compatibility test case?

@monsieurtanuki
Copy link
Contributor

@bhattabhi013 Try the dev mode and "Switch between strong and lenient matching" as "strong". With the lenient there's never an incompatibility.

@teolemon teolemon changed the title fix: color issue in light mode fix: color issue in light mode on the top summary bar of the scan card Apr 26, 2022
@bhattabhi013
Copy link
Contributor Author

Hi,
Thanks, @monsieurtanuki, and @M123-dev.
Here are the screenshots:

Light Mode:
image

Dark Mode:
image

This approach fixes issues in light mode but creates a new issue in dark mode.
I suggest changing the unknown compatibility color to the primary blue color. What do you suggest?
image

@monsieurtanuki
Copy link
Contributor

@bhattabhi013 Thank you for the screenshots.

In my opinion there's confusion here with ProductCompatibilityHelper.getBackgroundColor:

  • that is used for the little button before the compatibility label in the smooth card
  • that is also used for the summary card header with currently white text
  • we should probably use a different color for both use cases - a little button and a title background
  • we should probably use a different color for light and dark mode
  • I personally have a tendency to believe that we should stick to the mode and therefore write in dark on light for light mode and write in light on dark on dark mode

This is what I suggest in ProductCompatibilityHelper:

  • get rid of getBackgroundColor
  • Color getHeaderBackgroundColor(bool darkMode) {switch (matchedProduct.status) {... - with LIGHT_GREY_COLOR, LIGHT_GREEN_COLOR and LIGHT_RED_COLOR for light mode and very dark versions for dark mode
  • Color getHeaderForegroundColor(bool darkMode) => darkMode ? Colors.white : Colors.black
  • Color getButtonColor(bool darkMode) {switch (matchedProduct.status) {... - with fairly dark grey green red for light mode, and rather light versions for dark mode

What do you think of that?

@bhattabhi013
Copy link
Contributor Author

@monsieurtanuki,
LGTM, I also support the idea to write in dark for light mode and write in light on dark mode.
I'll update the changes with the latest commits.

@bhattabhi013
Copy link
Contributor Author

Hi @monsieurtanuki,
I have implemented the changes discussed.

Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

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

Hi @bhattabhi013!
Looks better that way but still some fine-tunings to code.

Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

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

Thank you @bhattabhi013!
I don't know if in the end we'll keep exactly those color values, but anyway now we are ready to handle all the cases.

@monsieurtanuki
Copy link
Contributor

@bhattabhi013 Not your fault: you'll need to merge #1772 once it's merged, without that the tests don't pass.

For the record, please don't reply to all my comments with "Changes done.": each time it's another email that I receive :(
Just a 👍 would do the trick - of course if you have something more challenging to say that makes sense to send a detailed message.

@teolemon
Copy link
Member

teolemon commented May 8, 2022

thank you very much @bhattabhi013 @monsieurtanuki 👌 merging

@teolemon teolemon merged commit b88f686 into openfoodfacts:develop May 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Color issues for compatibility unknown in light mode
5 participants