-
Notifications
You must be signed in to change notification settings - Fork 87
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
feat: add translations for Persian language #271
feat: add translations for Persian language #271
Conversation
Thanks for the pull request, @FatemeKhodayari! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
5c114a8
to
c55e1a9
Compare
Hi @FatemeKhodayari! Thank you for your contribution! I'm lining the changes up for a test run. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #271 +/- ##
==========================================
+ Coverage 96.04% 96.36% +0.32%
==========================================
Files 194 194
Lines 1844 1843 -1
Branches 322 326 +4
==========================================
+ Hits 1771 1776 +5
+ Misses 68 62 -6
Partials 5 5 ☔ View full report in Codecov by Sentry. |
@FatemeKhodayari Looks like we've got a green build! I'm marking this PR as ready for review, but let me know if you were planning on making further changes to it. |
Thanks for the checks and feedback. Although the edx-platform project on transifex is stopped and edx-translations does not have Persian yet, I can check transifex for possible updates if you think it's needed. But aside from that, don't think I have anything to add for now. Shall I sync everything with transifex or is it fine the way it is? |
Thanks for the details @FatemeKhodayari. I don't have enough context to answer your question, which means that now would definitely be a good time to get engineering review started :) @leangseu-edx Would you be able to take a look at this PR? |
Hi @viktorrusakov, I see that you're listed as a core contributor for all For context, one of the decisions made at the first meeting of the new Maintenance WG last week was that we can let CCs take on Reviews and Merges that they can manage. That will hopefully help us get reviews for community contributions more quickly, and is why I'm pinging you here :) |
HI @itsjeyd! You can absolutely tag me on PRs that need help with reviews, but I'm not sure I'll be able to help much with this one as I don't know what is the current process for adding a new language to MFEs. The code changes look good to me and I see that these translations are present in the Transifex resourse for this MFE, which means they will get updated automatically. I think this should be enough for this PR to get merged but I also know that there is an ongoing (maybe even finished already?) work on changing translations pipelines in repositories, so maybe there are additional steps we need to do which I'm not aware of. @brian-smith-tcril could you help with this PR? I think you were heavily involved in changing translations' workflows project. Just want to make sure this is OK to merge. |
Hi guys. I just merged master into my branch to keep it updated but then I realised that the approved workflows have gone back to "awaiting approval" state. Hope I haven't disrupted the process. Generally, what is the openedx policy for this situation? Should the developer keep the branch updated with master at all stages before merge? |
Hey all! I'm responding on behalf of the Aperture team just to say that I've got eyes on this as well, but since our team is no longer responsible for translations I can't confidently speak on what the new translation system requires for new languages. What I can do is at least make this PR known in the openedx #wg-translations channel, and hopefully we can get this merged sooner than later! |
@FatemeKhodayari Thanks for your contribution. Unfortunately, at the moment we don't support this workflow anymore and we're removing all the languages from Micro-frontends. Please see the related removal pull request: Since you've already merged that in your master I suggest keeping those change in your fork until Redwood in which a new approach will be used: You're welcome to start testing the experimantal Please take a look at the OEP-58 proposal which has been implemented and scheduled to be released in Redwood. I recommend closing this pull request. |
Hi Omar. Thanks for the feedback. Merging and getting all translations from a single source sounds like a good idea to me. Thanks for letting me know.
Actually I haven't merged these changes into the master branch of my fork, I have them in a separate branch. I merged the remote master into this branch to keep it updated but that doesn't matter now :)) About |
Yes @FatemeKhodayari. Running This is a Redwood feature which is being merged as we speak: |
So, I guess from now on, only translations from Someone requested Persian (fa-ir) some time ago in the |
Yes.
Please ping @ehuthmacher about the list of the languages we'll be adding. I believe the plan is to add about 100 more language including Persian. cc: @shadinaif |
@FatemeKhodayari Just checking in about next steps. Are you planning on closing this PR as @OmarIthawi suggested above? |
Yes sure. Hope Persian is added to the |
@FatemeKhodayari Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future. |
@FatemeKhodayari, yes, we will add Persian to the openedx-translations in TX this week. |
Hello @ehuthmacher! That's great news! Thanks a lot :) |
Translations for Persian (fa-ir language code) are added so the learner dashboard MFE will be usable for Persian users.
Closes #270