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: replace hardcoded strings and properly define i18n messages #219

Merged
merged 1 commit into from
Nov 16, 2023

Conversation

brian-smith-tcril
Copy link
Contributor

resolves #217

@codecov
Copy link

codecov bot commented Oct 13, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8f2ed77) 96.46% compared to head (10b5d7a) 96.46%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #219   +/-   ##
=======================================
  Coverage   96.46%   96.46%           
=======================================
  Files         195      195           
  Lines        1839     1839           
  Branches      322      322           
=======================================
  Hits         1774     1774           
  Misses         60       60           
  Partials        5        5           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@arbrandes arbrandes added the needs maintainer attention Issue or PR specifically needs the attention of the maintainer. label Oct 17, 2023
@hurtstotouchfire hurtstotouchfire added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Oct 18, 2023
@brian-smith-tcril
Copy link
Contributor Author

@hurtstotouchfire was the "waiting on author" label added because this needed a rebase?

@cintnguyen
Copy link
Contributor

@brian-smith-tcril Tested this locally, I believe it needs to be rebased off of master once the painted door PR is merged first. Otherwise it needs to include the EXPERIMENT_08_23_VAN_PAINTED_DOOR=true in order for the page to render.

After doing this on my end, ran make extract_translations and all three missing strings showed up in the transifix_input.json file generated. So this LGTM once it includes the env dev variable!

Side Question - Noticed that our layout is flipped when switching to the locale ar. Is this suppose to be the case?

@brian-smith-tcril
Copy link
Contributor Author

Side Question - Noticed that our layout is flipped when switching to the locale ar. Is this suppose to be the case?

Yes! Glad you tested that, now we know that Right-to-Left languages are working properly!

Copy link
Contributor

@cintnguyen cintnguyen left a comment

Choose a reason for hiding this comment

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

Tested this locally, ran make extract_translations and all three missing strings showed up in the transifix_input.json file generated. So this LGTM!

@justinhynes justinhynes removed the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Nov 8, 2023
@justinhynes
Copy link
Contributor

@brian-smith-tcril Hey there! This looks good to me but I can't seem to merge the changes with my current permissions.

It looks like we may need to rebase these changes on the base/main branch before I can merge it.

If you could do that, I'll plan on merging it shortly afterwards.

(If I have write access, should I be able to automatically rebase the branch through GitHub? Maybe this feature isn't enabled on this repo?)

@jsnwesson jsnwesson added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Nov 13, 2023
@arbrandes
Copy link
Contributor

@brian-smith-tcril, mind rebasing?

@brian-smith-tcril
Copy link
Contributor Author

@arbrandes rebased, ci passed

@arbrandes arbrandes merged commit c2a20af into openedx:master Nov 16, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs maintainer attention Issue or PR specifically needs the attention of the maintainer. waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

i18n issues in frontend-app-learner-dashboard
6 participants