Skip to content

Add non-English prop and grades translations in Curriculum Catalog Card - #51302

Merged
megcrenshaw merged 14 commits into
stagingfrom
meg/add-locales-curriculum-catalog
Apr 17, 2023
Merged

Add non-English prop and grades translations in Curriculum Catalog Card#51302
megcrenshaw merged 14 commits into
stagingfrom
meg/add-locales-curriculum-catalog

Conversation

@megcrenshaw

@megcrenshaw megcrenshaw commented Apr 12, 2023 •
edited
Loading

Copy link
Copy Markdown

This PR does three things:

  1. Pass in hard-coded isTranslated and duration props (follow-up from previous PR)

  2. Pass in isEnglish prop from backend using LocaleHelper

Spanish:
image

English UK:
image

  1. Add capability for one grade level to be shown.

Single Grade:
image

More than one grade:
image

Links

Testing story

  • For the grade level validation, I made sure all of our existing grade levels are valid:
[development] dashboard > CourseOffering.last.update!(grade_levels: 'invalid')
  CourseOffering Load (0.6ms)  SELECT `course_offerings`.* FROM `course_offerings` ORDER BY `course_offerings`.`id` DESC LIMIT 1
Traceback (most recent call last):
        1: from (irb):2
ActiveRecord::RecordInvalid (Validation failed: Grade levels must be comma-separated values with optional K first and digits)
[development] dashboard > CourseOffering.all.each(&:save!)
  • Added a unit test for one grade.
  • Updated storybook to show one grade.
  • Tested the localization manually (see screenshots).
  • Not sure it makes sense to add unit tests for all the different permutations of isEnglish props, but happy for more conversation here.

Deployment strategy

Follow-up work

Privacy

Security

Caching

PR Checklist:

  • Tests provide adequate coverage
  • Privacy and Security impacts have been assessed
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@megcrenshaw
megcrenshaw force-pushed the meg/add-locales-curriculum-catalog branch from 91a6101 to 614af92 Compare April 13, 2023 16:04
@megcrenshaw
megcrenshaw marked this pull request as ready for review April 13, 2023 19:44
@megcrenshaw
megcrenshaw requested a review from a team as a code owner April 13, 2023 19:44
@megcrenshaw
megcrenshaw requested a review from a team April 14, 2023 15:00
davidsbailey
davidsbailey approved these changes Apr 14, 2023
View reviewed changes

@davidsbailey davidsbailey left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice work Meg! LGTM once requested test cases are added and passing.

regarding plurals, I have ideas here (which i've shared previously) but am not an expert on the actual capabilities of our current system. I would recommend reaching out to the platform team to get the info you need here. I see you tagged foundations, but they get a lot of PRs (anything tagged with i18n), so don't hesitate to also reach out via slack.

Comment thread apps/src/templates/curriculumCatalog/CurriculumCatalogCard.jsx Outdated
youngestGrade: gradesArray[0],
oldestGrade: gradesArray[gradesArray.length - 1]
})
}

@davidsbailey davidsbailey Apr 14, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the overall approach for how to handle grading data seems good to me 👍

Comment thread dashboard/test/models/course_offering_test.rb
assert_creates CourseOffering do
CourseOffering.create!(key: 'test-key-one-grade', display_name: 'Test One Grade', grade_levels: 'K')
end
end

@davidsbailey davidsbailey Apr 14, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

thank you for the excellent validation and tests! grades seem like a surprisingly annoying problem for something which seems so simple at first, so this effort seems well spent.

a couple more test cases to throw at you:

  • '10,11,12'
  • a couple other passing test cases, reflecting common uses
  • 'K,K,1'

@megcrenshaw megcrenshaw Apr 14, 2023

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Oh the K,K,1 is clever! Just added more tests

@davidsbailey davidsbailey Apr 14, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

new tests look great 👍

@megcrenshaw

megcrenshaw commented Apr 14, 2023 •
edited
Loading

Copy link
Copy Markdown
Author

Nice work Meg! LGTM once requested test cases are added and passing.

regarding plurals, I have ideas here (which i've shared previously) but am not an expert on the actual capabilities of our current system. I would recommend reaching out to the platform team to get the info you need here. I see you tagged foundations, but they get a lot of PRs (anything tagged with i18n), so don't hesitate to also reach out via slack.

Good thing you asked about reaching out to the platform team! I had done that https://codedotorg.slack.com/archives/CFTFD6BPV/p1681305819638809 but still used the wrong documentation, which was why nothing was matching / working! Got it working now 🎉

UPDATE: See

code-dot-org/apps/i18n/fish/ga_ie.json

Line 51 in 79e0ce0

"fishlong-pond-init1": "Bunaithe ar an traenáil a rinne tú, bhí AI in ann {n, plural,one {an t-iasc seo} two {na héisc seo} few {na héisc seo} many {na héisc seo} other {na héisc seo}} a aithint mar “{word}”. Conas a d'éirigh léi?",
for how other languages will translate with more plural options

mgc1194
mgc1194 reviewed Apr 14, 2023
View reviewed changes

@mgc1194 mgc1194 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks great.
I checked on Crowdin and the plural formatting looks great.
Screenshot 2023-04-14 at 15 37 25

daynew
daynew reviewed Apr 15, 2023
View reviewed changes
Comment thread apps/i18n/common/en_us.json
Comment on lines 1097 to +1098
"grade": "Grade",
"gradeRange": "Grades: {youngest_grade}-{oldest_grade}",
"gradeRange": "{numGrades, plural, one {Grade: {youngestGrade}} other {Grades: {youngestGrade}-{oldestGrade}}}",

@daynew daynew Apr 15, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't expect you to change anything since this is probably a core issue with our platform, but this is just a thought I had: I imagine "Age" would be a more universal attribute across cultures rather than the specific US education concept of a "Grade".

@megcrenshaw megcrenshaw Apr 17, 2023

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, good call. We'll use the user's country to determine if we show age (outside the US) or grade (inside the US), but that's happening in version 2.

@megcrenshaw
megcrenshaw merged commit 7ab78ef into staging Apr 17, 2023
@megcrenshaw
megcrenshaw deleted the meg/add-locales-curriculum-catalog branch April 17, 2023 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@daynew daynew daynew left review comments
@davidsbailey davidsbailey davidsbailey approved these changes
+1 more reviewer
@mgc1194 mgc1194 mgc1194 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@megcrenshaw @daynew @davidsbailey @mgc1194