Skip to content

i18n Issues Mega Issue part 2 (Controversial part)#1922

Merged
bradenmacdonald merged 7 commits intoopenedx:masterfrom
jacobo-dominguez-wgu:fix/issue-1806-i18n-controversial
May 13, 2025
Merged

i18n Issues Mega Issue part 2 (Controversial part)#1922
bradenmacdonald merged 7 commits intoopenedx:masterfrom
jacobo-dominguez-wgu:fix/issue-1806-i18n-controversial

Conversation

@jacobo-dominguez-wgu
Copy link
Copy Markdown
Contributor

@jacobo-dominguez-wgu jacobo-dominguez-wgu commented May 7, 2025

Description

Part 2 of solving i18n issues containing the controversial section. Link to part 1
Based on a report generated by an AI agent about improvements to untranslated strings according to the i18 guidelines, several files have been modified in order to fix these potential internationalization bugs.
Some of the fixes include:

  • Untranslated aria-label texts

Supporting information

This PR fixes #1806

Testing instructions

  • CardHeader
    Go to the edit course content.
    Hover a module name and click on the pencil icon that shows on hover, the module name turns into an input element
    With devtools inspect the input text and look for the aria-label (it is a bit difficult since clicking turns the input into a label, so here is helpful to install the react developer tools and look for the specific component (which is FormControl) on the Components tool)

  • AnswerWidget/AnswerOption
    Select one of the sections of a module and click on the arrow to expand the content.
    Click on + New unit.
    Select Problem component and then the single select option and click select button.
    Scroll down the modal and there must be the Answers section.
    Look for the button with an icon of a comment with an exclamation mark and inspect it.
    On the devtools look for the aria-label of the parent div.

  • LicenseWidget/LicenseDetails
    Select one of the sections of a module and click on the arrow to expand the content.
    Click on + New unit.
    On the add new component section select Video option.
    Scroll down to the License section and click + Add a license for this video.
    On License Type select Creative Commons.
    Scroll down to the License Details and inspect the checkboxes for the list options.
    With devtools inspect the aria-label for all the checkboxes.

  • FileInput
    TBD.

  • SortAndFilterModal
    Go to the assets page for a course: http://apps.local.openedx.io:2001/authoring/course/{courseId}/assets
    Click on the sort and filter button
    On the modal, inspect the options on the Sort by section
    Inspect the aria-label for each element

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label May 7, 2025
@openedx-webhooks
Copy link
Copy Markdown

Thanks for the pull request, @jacobo-dominguez-wgu!

This repository is currently maintained by @openedx/2u-tnl.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

  • If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
    • This process (including the steps you'll need to take) is documented here.
  • If it doesn't, simply proceed with the next step.
🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

  • Dependencies

    This PR must be merged before / after / at the same time as ...

  • Blockers

    This PR is waiting for OEP-1234 to be accepted.

  • Timeline information

    This PR must be merged by XX date because ...

  • Partner information

    This is for a course on edx.org.

  • Supporting documentation
  • Relevant Open edX discussion forum threads
🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Details
Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

  • The size and impact of the changes that it introduces
  • The need for product review
  • Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

@codecov
Copy link
Copy Markdown

codecov bot commented May 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.79%. Comparing base (d717303) to head (e22b1d6).
Report is 17 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff            @@
##           master    #1922    +/-   ##
========================================
  Coverage   93.78%   93.79%            
========================================
  Files        1147     1154     +7     
  Lines       23907    24083   +176     
  Branches     5148     5100    -48     
========================================
+ Hits        22422    22589   +167     
- Misses       1408     1426    +18     
+ Partials       77       68     -9     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jacobo-dominguez-wgu jacobo-dominguez-wgu changed the title Fix/issue 1806 i18n controversial i18n Issues Mega Issue part 2 (Controversial part) May 7, 2025
@jacobo-dominguez-wgu jacobo-dominguez-wgu moved this from Needs Triage to Ready for Review in Contributions May 7, 2025
@mphilbrick211 mphilbrick211 requested a review from a team May 9, 2025 17:33
@mphilbrick211 mphilbrick211 added the mao-onboarding Reviewing this will help onboard devs from an Axim mission-aligned organization (MAO). label May 12, 2025
noneAriaLabel: {
id: 'course-authoring.video-uploads.transcriptSettings.orderTranscripts.none.aria-label',
defaultMessage: 'none radio',
description: 'Aria label for order transcript None option',
Copy link
Copy Markdown
Contributor

@bradenmacdonald bradenmacdonald May 13, 2025

Choose a reason for hiding this comment

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

Will translators know the difference between 'Label for order transcript None option' and 'Aria label for order transcript None option'?

It might be helpful to say "Accessible (screen reader) label for 'order transcript: None' option, indicating this is a radio button (mutually exclusive selection)". But just thinking out loud here, it's also fine as is.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes I agree, will update all descriptions for aria label.

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.

Hey, I see you requested a re-review but these descriptions aren't updated yet?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sorry I missed these.

@jacobo-dominguez-wgu jacobo-dominguez-wgu force-pushed the fix/issue-1806-i18n-controversial branch from 83f0543 to dab2b1a Compare May 13, 2025 16:07
@jacobo-dominguez-wgu jacobo-dominguez-wgu force-pushed the fix/issue-1806-i18n-controversial branch from dab2b1a to 4ab1399 Compare May 13, 2025 16:27
Copy link
Copy Markdown
Contributor

@bradenmacdonald bradenmacdonald left a comment

Choose a reason for hiding this comment

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

Thanks, I tested as described and it's working well. Just let me know if you are planning any of the remaining "Aria label..." descriptions before I go ahead and merge it.

axiosMock.onGet(`${getAssetsUrl(courseId)}?display_name=download.png&page_size=1`).reply(200, { assets: [] });
axiosMock.onPost(getAssetsUrl(courseId)).reply(200, generateNewAssetApiResponse());
const addFilesButton = screen.getByLabelText('file-input');
const addFilesButton = screen.getByLabelText(messages.fileInputAriaLabel.defaultMessage);
Copy link
Copy Markdown
Contributor

@bradenmacdonald bradenmacdonald May 13, 2025

Choose a reason for hiding this comment

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

Suggested change
const addFilesButton = screen.getByLabelText(messages.fileInputAriaLabel.defaultMessage);
const addFilesButton = screen.getByLabelText('Upload a file');

Nit: This may seem to go against the usual "DRY" developer maxim, but I generally prefer that strings are hard-coded in test files, because it makes it much more clear when things get changed accidentally. When the test references the code, it's easier to introduce a regression without noticing. In this case it's not a big deal though. Feel free to leave it as is.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I understand your point but lets keep it like this this time since it is not a major thing.

noneAriaLabel: {
id: 'course-authoring.video-uploads.transcriptSettings.orderTranscripts.none.aria-label',
defaultMessage: 'none radio',
description: 'Aria label for order transcript None option',
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.

Hey, I see you requested a re-review but these descriptions aren't updated yet?

<FormattedMessage {...messages.detailsSubsectionTitle} />
</div>
{license === LicenseTypes.allRightsReserved ? (
{license === LicenseTypes.allRightsReserved && (
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.

Just curious, why are you replacing the ternary operators?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This one was a suggestion from a fellow, so the code looks cleaner and more readable.

@bradenmacdonald bradenmacdonald enabled auto-merge (squash) May 13, 2025 19:06
@bradenmacdonald bradenmacdonald merged commit 88aa4c1 into openedx:master May 13, 2025
7 checks passed
@github-project-automation github-project-automation bot moved this from Ready for Review to Done in Contributions May 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

mao-onboarding Reviewing this will help onboard devs from an Axim mission-aligned organization (MAO). open-source-contribution PR author is not from Axim or 2U

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

i18n Issues Mega Issue

4 participants