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.

Sign up for GitHub

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

Jump to bottom

feat: Better support RTL languages #4310

Merged
merged 4 commits into from
Jul 16, 2023
Merged

feat: Better support RTL languages #4310

merged 4 commits into from
Jul 16, 2023

Conversation

g123k
Copy link
Collaborator

@g123k g123k commented Jul 14, 2023

Hi everyone,

We often use left/right values, where we should instead pass start/end.
This PR is a simple find/replace.

Nothing interesting

@g123k g123k added the RTL Working well in right to left languages like Arabic or Hebrew label Jul 14, 2023
@g123k g123k self-assigned this Jul 14, 2023
@g123k g123k requested a review from a team as a code owner July 14, 2023 19:03
@github-actions github-actions bot added 🥫 Product page Product addition The easier it is to add a product and get Nutri-Score, Eco-Score, the happier the users. 🤗 Onboarding We need to onboard users on how the app works, but also on content like Nutri-Score, Eco-Score… 📈 Analytics We use Sentry and Matomo, with an opt-in system 👥 User management Account login, signup, signout 🤳🥫 Scan We need to be able to scan on low-end, old devices, even with a bad camera, connexion… ✏️ Editing - Packaging input Related to the structured input of food packaging. User lists labels Jul 14, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jul 14, 2023 •
edited
Loading

Codecov Report

Merging #4310 (3c230c7) into develop (1cd22a6) will decrease coverage by 0.01%.
The diff coverage is 31.25%.

@@             Coverage Diff             @@
##           develop    #4310      +/-   ##
===========================================
- Coverage    10.80%   10.80%   -0.01%     
===========================================
  Files          287      287              
  Lines        14202    14203       +1     
===========================================
  Hits          1534     1534              
- Misses       12668    12669       +1
Impacted Files Coverage Δ
...pp/lib/cards/product_cards/product_title_card.dart 0.00% <ø> (ø)
...ds/product_cards/smooth_product_card_template.dart 2.27% <ø> (ø)
...owledge_panels/knowledge_panel_world_map_card.dart 0.00% <ø> (ø)
...p/lib/pages/onboarding/consent_analytics_page.dart 0.00% <0.00%> (ø)
...oth_app/lib/pages/onboarding/country_selector.dart 0.82% <ø> (ø)
...s/smooth_app/lib/pages/onboarding/next_button.dart 4.54% <0.00%> (ø)
...pp/lib/pages/onboarding/onboarding_bottom_bar.dart 0.00% <0.00%> (ø)
...oth_app/lib/pages/onboarding/permissions_page.dart 0.00% <0.00%> (ø)
...mooth_app/lib/pages/personalized_ranking_page.dart 0.00% <ø> (ø)
...b/pages/preferences/user_preferences_settings.dart 6.31% <ø> (ø)
... and 12 more

... and 2 files with indirect coverage changes

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

monsieurtanuki
monsieurtanuki approved these changes Jul 15, 2023
View reviewed changes
packages/smooth_app/lib/generic_lib/dialogs/smooth_alert_dialog.dart Outdated
Comment on lines 89 to 96
final double topPadding;
if (padding is EdgeInsetsDirectional) {
topPadding = padding.bottom;
} else if (padding is EdgeInsets) {
topPadding = padding.bottom;
} else {
topPadding = 0.0;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Cannot the point of this: there's only one place this method is called, and with a fat chance we need padding.bottom anyway.
Maybe I'm missing something here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would like to force EdgeInsetsDirectional everywhere, but here indeed we have still strange behavior.

Before reverting this part, what do you think about forcing all EdgeInsets to be EdgeInsetsDirectional and enforcing this with a Lint rule?

Copy link
Contributor

Choose a reason for hiding this comment

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

For the moment just make padding an EdgeInsetsDirectional, which will give you access to .bottom and enable you to remove your tests.

Before reverting this part, what do you think about forcing all EdgeInsets to be EdgeInsetsDirectional and enforcing this with a Lint rule?

If it's easy to you to code and easy for us to maintain, why not.
But it's not a priority.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't know why I've overengineered this part.
That should be better now.

For the Lint part, yes indeed, not a priority

@github-actions
Copy link
Contributor

You can test this PR on: https://app.waldo.com/applications/app-19d476740ba1bb36/sessions

@g123k g123k merged commit b03f60b into openfoodfacts:develop Jul 16, 2023
7 checks passed
@g123k g123k deleted the rtl_padding branch July 16, 2023 18:29
@github-actions github-actions bot mentioned this pull request Jul 16, 2023
Merged
@g123k g123k mentioned this pull request May 31, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@monsieurtanuki monsieurtanuki monsieurtanuki approved these changes

Assignees

@g123k g123k

Labels
📈 Analytics We use Sentry and Matomo, with an opt-in system ✏️ Editing - Packaging input Related to the structured input of food packaging. 🤗 Onboarding We need to onboard users on how the app works, but also on content like Nutri-Score, Eco-Score… Product addition The easier it is to add a product and get Nutri-Score, Eco-Score, the happier the users. 🥫 Product page RTL Working well in right to left languages like Arabic or Hebrew 🤳🥫 Scan We need to be able to scan on low-end, old devices, even with a bad camera, connexion… User lists 👥 User management Account login, signup, signout
Projects
Right-to-Left support accross Open Food Facts
Status: No status
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@g123k @codecov-commenter @monsieurtanuki