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: #2291 - removed flawed specific font (back to default fonts) #2657

Merged
merged 1 commit into from
Jul 27, 2022

Conversation

monsieurtanuki
Copy link
Contributor

Deleted files:

  • OFL.txt
  • PlusJakartaSans-Bold.ttf
  • PlusJakartaSans-Regular.ttf

Impacted files:

  • main.dart: removed font licence registering
  • pubspec.yaml: removed specified fonts (back to default fonts)
  • smooth_theme.dart: removed specified fonts (back to default fonts)

What

  • Some non latin letters are not rendered at all on some devices (probably Android).
  • A couple of issues were created from users comment, about Greek and Ukrainian.
  • We used a specific font (JakartaSans)
  • When we switch back to the default flutter font, we don't have this problem anymore.
  • As the standard flutter font and the one we specified are very similar, there's no added value in trying to use a font that does not always work. Unless we enjoy getting 1 star review (I don't).

Screenshot

Ukraine/Ukrainian:

country/language default flutter font Jakarta font
Ukraine/Ukrainian Capture d’écran 2022-07-25 à 17 53 38 Capture d’écran 2022-07-25 à 17 51 54
US/Greek Capture d’écran 2022-07-25 à 17 38 18 Capture d’écran 2022-07-25 à 17 34 57

Fixes bug(s)

…lt fonts)

Deleted files:
* `OFL.txt`
* `PlusJakartaSans-Bold.ttf`
* `PlusJakartaSans-Regular.ttf`

Impacted files:
* `main.dart`: removed font licence registering
* `pubspec.yaml`: removed specified fonts (back to default fonts)
* `smooth_theme.dart`: removed specified fonts (back to default fonts)
@monsieurtanuki monsieurtanuki requested a review from a team as a code owner July 25, 2022 16:17
@codecov-commenter
Copy link

Codecov Report

Merging #2657 (ba2852b) into develop (2ea0da3) will decrease coverage by 1.55%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           develop   #2657      +/-   ##
==========================================
- Coverage     8.86%   7.31%   -1.56%     
==========================================
  Files          161     218      +57     
  Lines         6623   10491    +3868     
==========================================
+ Hits           587     767     +180     
- Misses        6036    9724    +3688     
Impacted Files Coverage Δ
...kages/smooth_app/lib/widgets/attribute_button.dart 0.00% <0.00%> (-92.00%) ⬇️
...s/smooth_app/lib/data_models/user_preferences.dart 8.73% <0.00%> (-23.57%) ⬇️
packages/smooth_app/lib/themes/smooth_theme.dart 60.00% <0.00%> (-22.98%) ⬇️
...p/lib/generic_lib/dialogs/smooth_alert_dialog.dart 15.11% <0.00%> (-19.10%) ⬇️
...mooth_app/lib/data_models/product_preferences.dart 21.68% <0.00%> (-9.75%) ⬇️
packages/smooth_app/lib/main.dart 14.65% <0.00%> (-3.24%) ⬇️
.../smooth_app/lib/pages/onboarding/welcome_page.dart 0.00% <0.00%> (-3.13%) ⬇️
.../smooth_app/lib/pages/onboarding/scan_example.dart 0.00% <0.00%> (-2.28%) ⬇️
...p/lib/pages/onboarding/consent_analytics_page.dart 0.00% <0.00%> (-1.57%) ⬇️
...ackages/smooth_app/lib/pages/scan/scan_header.dart 3.33% <0.00%> (-1.43%) ⬇️
... and 235 more

Help us with your feedback. Take ten seconds to tell us how you rate us.

@monsieurtanuki monsieurtanuki linked an issue Jul 26, 2022 that may be closed by this pull request
@teolemon
Copy link
Member

I guess it is the right thing to do, but we're loosing the unique identity
People seem to be complaining:

We should probably ask Quentin for a similar font replacement. Meanwhile, is there a way to disable it only for non-latin languages ?

@monsieurtanuki
Copy link
Contributor Author

@teolemon What about merging this PR first, and find the bullet-proof font / the perfect font combo later?

I'm hyper sensitive to 1-star ratings, and if I think about the time and energy spent on the rest of the app, I find it a bit depressing to have users reject the app for legitimate reasons that would require an easy fix.

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.

The changes look good and I totally agree with the 1 Star reviews

@monsieurtanuki monsieurtanuki merged commit 180c817 into openfoodfacts:develop Jul 27, 2022
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.

Issue with Greek version of the app Encoding issue in Ukrainian
4 participants