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

Persian language #246

Merged
merged 6 commits into from
Sep 11, 2023
Merged

Persian language #246

merged 6 commits into from
Sep 11, 2023

Conversation

CodeWithEmad
Copy link
Member

@CodeWithEmad CodeWithEmad commented Aug 8, 2023 •
edited
Loading

This will add Persian (fa_IR) language to the Ora Grading MFE.
Additionally, a few typos have been corrected.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Aug 8, 2023
@openedx-webhooks
Copy link

openedx-webhooks commented Aug 8, 2023 •
edited
Loading

Thanks for the pull request, @CodeWithEmad! 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:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

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.

@mphilbrick211 mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Aug 9, 2023
@e0d
Copy link
Contributor

e0d commented Aug 16, 2023

@CodeWithEmad looks like there are some test failures, can you have a look?

@e0d e0d removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Aug 16, 2023
@CodeWithEmad
Copy link
Member Author

sure @e0d. Fixed and tested on my local.
Can you please rerun the tests?

@mphilbrick211 mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Aug 17, 2023
@codecov
Copy link

codecov bot commented Aug 23, 2023 •
edited
Loading

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (358263d) 100.00% compared to head (1737dc9) 100.00%.

❗ Current head 1737dc9 differs from pull request most recent head 9691e96. Consider uploading reports for the commit 9691e96 to get more accurate results

Additional details and impacted files 
@@            Coverage Diff            @@
##            master      #246   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          110       110           
  Lines         1079      1079           
  Branches       159       159           
=========================================
  Hits          1079      1079
Files Changed Coverage Δ
src/containers/CriterionContainer/messages.js 100.00% <ø> (ø)
src/containers/ResponseDisplay/messages.js 100.00% <ø> (ø)
src/containers/Rubric/messages.js 100.00% <ø> (ø)
src/data/redux/grading/reducer.js 100.00% <ø> (ø)
src/data/redux/grading/selectors/nav.js 100.00% <ø> (ø)
src/data/redux/thunkActions/grading.js 100.00% <ø> (ø)
src/data/redux/thunkActions/requests.js 100.00% <ø> (ø)
src/data/services/lms/api.js 100.00% <ø> (ø)
src/data/utils.js 100.00% <ø> (ø)
src/i18n/index.js 100.00% <ø> (ø)
... and 1 more

... and 1 file with indirect coverage changes

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

@e0d e0d removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Aug 23, 2023
@mphilbrick211 mphilbrick211 requested a review from a team August 24, 2023 15:01
@mphilbrick211
Copy link

Hi @openedx/content-aurora! Would someone be able to please review/merge this for us? Thanks!

OmarIthawi
OmarIthawi suggested changes Sep 8, 2023
View reviewed changes
Copy link
Member

@OmarIthawi OmarIthawi left a comment

Choose a reason for hiding this comment

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

Thanks @CodeWithEmad again and appologies for the late review requesting changes.

Same here please. The commit chore: Persian translations added which adds the translations .json file should be removed.

I've only skimmed through the typos, but overall the pull request looks good.

@CodeWithEmad
Copy link
Member Author

@OmarIthawi I removed other language translations.
As I mentioned in our Slack chat, If you want to introduce a new language to any of the repos, you have to add the LANGUAGE_CODE.json file. otherwise how you can import it into the src/i18n/index.js? tests (specifically lint) will fail if you do so.

8:26 error Unable to resolve path to module './messages/fa_IR.json' import/no-unresolved

leangseu-edx
leangseu-edx approved these changes Sep 8, 2023
View reviewed changes
Copy link
Contributor

@leangseu-edx leangseu-edx left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@CodeWithEmad
Copy link
Member Author

@leangseu-edx your last commit here has conflicts with the master branch.

OmarIthawi
OmarIthawi reviewed Sep 8, 2023
View reviewed changes
src/i18n/messages/fa_IR.json Outdated
@@ -0,0 +1,105 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

I recommend adding a empty file here. This file will be overridden by the translations bot anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

:) sure

Copy link
Member Author

@CodeWithEmad CodeWithEmad Sep 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Persian translations were removed. should I remove the conflicted commit @leangseu-edx added?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about the package.json files. But generally, the preference is to make it in a separate pull request to make your contributions atomic.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree, @leangseu-edx add that commit. I'll remove package.json and package-lock.json from his commit and this should be ready to review.

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry, for late response. The reason I update the package was that this pr wasn't compile locally.

@CodeWithEmad CodeWithEmad force-pushed the persian-language branch 2 times, most recently from e1a12ee to 9472a7f Compare September 9, 2023 14:42
CodeWithEmad and others added 6 commits September 9, 2023 18:19
@CodeWithEmad
fix: corrected typos
bcad1c3 
explaination -> explanation
Critera -> Criteria
addtional -> additional
arbitary -> arbitrary
penging -> pending
downladFiles -> downloadFiles
isLoadeed -> isLoaded
selectror -> selector
commnents -> comments
stirng -> string
isGragrding -> isGrading
queu -> queue
seleted -> selected
feecback -> feedback
@CodeWithEmad
feat: Persian language (fa_IR) added
9569263
@CodeWithEmad
chore: Persian translations added
9050467
@CodeWithEmad
chore: sort language codes alphabetically
979143f
@CodeWithEmad
chore: camelCase variable used for translated messages
fdb0f63
@leangseu-edx @CodeWithEmad
chore: deprecated styling updated
9691e96
OmarIthawi
OmarIthawi approved these changes Sep 9, 2023
View reviewed changes
Copy link
Member

@OmarIthawi OmarIthawi left a comment

Choose a reason for hiding this comment

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

Thanks @CodeWithEmad. i18n changes looks great! Thanks again for your contribution.

@leangseu-edx leangseu-edx merged commit e25a5a9 into openedx:master Sep 11, 2023
@openedx-webhooks
Copy link

@CodeWithEmad 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leangseu-edx leangseu-edx leangseu-edx approved these changes

@OmarIthawi OmarIthawi OmarIthawi approved these changes

Assignees
No one assigned
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Contributions
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@CodeWithEmad @openedx-webhooks @e0d @mphilbrick211 @OmarIthawi @leangseu-edx