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: Transportation Component Icon and title color is not compatible #1494

Merged
merged 16 commits into from
Apr 9, 2022
Merged

fix: Transportation Component Icon and title color is not compatible #1494

merged 16 commits into from
Apr 9, 2022

Conversation

bhattabhi013
Copy link
Contributor

What

fixes Transportation Component Icon and title color is not compatible with the background color.

Screenshot

image

Fixes bug(s)

Part of

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!
Please have a look at my comments, and attach a screenshot in both dark and light mode.

@bhattabhi013
Copy link
Contributor Author

Hi @monsieurtanuki ,
Please find the updated screenshots:

Light Mode:
image

Night Mode:
image

@codecov-commenter
Copy link

codecov-commenter commented Apr 5, 2022

Codecov Report

Merging #1494 (a81a260) into develop (c02a37c) will decrease coverage by 0.15%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           develop   #1494      +/-   ##
==========================================
- Coverage     9.06%   8.90%   -0.16%     
==========================================
  Files          158     161       +3     
  Lines         6422    6581     +159     
==========================================
+ Hits           582     586       +4     
- Misses        5840    5995     +155     
Impacted Files Coverage Δ
...s/knowledge_panels/knowledge_panel_title_card.dart 0.00% <0.00%> (ø)
...mooth_app/lib/data_models/product_query_model.dart 1.38% <0.00%> (-0.62%) ⬇️
packages/smooth_app/lib/pages/page_manager.dart 2.12% <0.00%> (-0.32%) ⬇️
...mooth_app/lib/pages/user_preferences_dev_mode.dart 0.69% <0.00%> (-0.10%) ⬇️
...kages/smooth_app/lib/data_models/product_list.dart 1.85% <0.00%> (-0.08%) ⬇️
...smooth_app/lib/helpers/smooth_matched_product.dart 1.44% <0.00%> (-0.07%) ⬇️
...ib/pages/onboarding/onboarding_flow_navigator.dart 1.61% <0.00%> (-0.03%) ⬇️
...smooth_app/lib/pages/product/new_product_page.dart 0.87% <0.00%> (-0.02%) ⬇️
...ages/smooth_app/lib/database/dao_product_list.dart 0.00% <0.00%> (ø)
...ges/smooth_app/lib/generic_lib/loading_dialog.dart 0.00% <0.00%> (ø)
... and 39 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 c02a37c...a81a260. Read the comment docs.

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.

@bhattabhi013 My comments:

  • what is the reason behind removing CHANGELOG.md?
  • I don't think your screenshot in dark mode reflect your latest code, as I don't expect blue in that case
  • I don't expect blue either, unless it was the color of the mock-ups

@bhattabhi013
Copy link
Contributor Author

bhattabhi013 commented Apr 5, 2022

Hi @monsieurtanuki,

Color? _getColorFromEvaluation(Evaluation evaluation) {
    switch (evaluation) {
      case Evaluation.BAD:
        return RED_COLOR;
      case Evaluation.NEUTRAL:  // (case which is used for the transportation icon and title in issue#1347)
        return PRIMARY_BLUE_COLOR;   
      case Evaluation.AVERAGE:
        return LIGHT_ORANGE_COLOR;
      case Evaluation.GOOD: 
        return LIGHT_GREEN_COLOR;
      case Evaluation.UNKNOWN: // (case where we used onBackground color in dark mode)
        return null;
    }
    
  • Please suggest a color for Evaluation.NEUTRAL case.

@monsieurtanuki
Copy link
Contributor

@bhattabhi013

  • please use the triple back-quote syntax when you're referring to multi-line code on github
  • there's a misunderstanding here - I have no specific relevancy regarding UI, I just made a remark on asset files and accidentally got mixed into this UI PR. And unfortunately I don't have that much time to deal with UI. And I don't know who's in charge of accessibility and colors - maybe @teolemon?

@bhattabhi013
Copy link
Contributor Author

Thanks for your inputs @monsieurtanuki.
@teolemon , Can you please help to decide on a color for this fix?

@VaiTon VaiTon requested a review from teolemon April 6, 2022 23:05
@stephanegigandet
Copy link
Contributor

on the website, we use the same grey color for both neutral and unknown.

$evaluation-good-color: #219653;
$evaluation-average-color: #f2994a;
$evaluation-bad-color: #eb5757;
$evaluation-unknown-color: #4f4f4f;

@stephanegigandet
Copy link
Contributor

Regarding a comment above (in one of the resolved discussion):

I don't think we have mocks for dark mode, but the whole point of dark mode is to have different colors, so the evaluation colors do not have to be (and probably should not be) exactly the same as the light mode colors.

My recommendation would be to create a set of light mode colors (for those you can use the ones from the website: they are the ones from the mocks) + a set of dark mode colors, which you can initially set to the light mode values (except the grey which should be lighter), and then we can ask the designer to select exactly the colors so that they are contrasted enough while not being too flashy.

@bhattabhi013
Copy link
Contributor Author

Hi @stephanegigandet,
Thanks for your input. Now, I'll proceed with the following tasks:

  1. Use a grey color for Evaluation.NEUTRAL case.
  2. Create a set of colors for light mode and another set for dark color, which will be similar to the light mode set except for the grey color case.

@bhattabhi013
Copy link
Contributor Author

As suggested by @stephanegigandet, I created 2 separate sets and they are used on the basis of light/dark mode.

Here are the updated screenshots:

  1. Light Mode

image

  1. Dark Mode

image

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!
We're getting close but still things to fix: please read my comments.

@bhattabhi013
Copy link
Contributor Author

@monsieurtanuki, Thanks for your comments and inputs. I've worked on the comments listed above and committed the changes, please let me know if any other modifications are required.

@VaiTon VaiTon requested a review from monsieurtanuki April 9, 2022 11:33
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 for your fixes.
Reading again and again the code, I think that evaluation == null is equivalent to UNKNOWN, and should be dealt similarly, color-wise. Please have a look at my comments.

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.

That's good @bhattabhi013!

@monsieurtanuki monsieurtanuki merged commit c5cf03b into openfoodfacts:develop Apr 9, 2022
@bhattabhi013
Copy link
Contributor Author

@monsieurtanuki, thanks for your support and guidance throughout this PR.

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.

Transportation Component Icon and title color is not compatible with background color.
5 participants