refactor: change lang selector logic #493

dcoa · 2025-01-13T01:45:08Z

Description and motivation

This PR refactors the lang selector logic recognizing its significance for multi-language instances.
The Open edX platform previously featured a configurable language selector in both the header and footer. However, after transitioning to microfrontends, this feature is only partially implemented in the footer. It requires passing a list of supported languages and a change function, necessitating additional modifications even when using the footer slot.

This PR change the dropdown and add the user preference update endpoint.

Proposal

Convert this component in a slot so could be installed in the header as well and give them the operators the option to use the language selector in the header or footer.

How to test

Override the Footer component in a MFE or use the example application to test the current implementation.
Add the configurations via env,config.js or MFE Config API.

const config = { ENABLE_FOOTER_LANG_SELECTOR: true, SITE_SUPPORTED_LANGUAGES: ['es', 'en'], };

You need the messages for the selected language if you want to visualize the change.
You can see the change in the language cookie.

2025-01-13.21-07-20.mov

openedx-webhooks · 2025-01-13T01:45:13Z

Thanks for the pull request, @dcoa!

This repository is currently maintained by @openedx/committers-frontend.

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.

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 · 2025-01-13T01:46:35Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.25%. Comparing base (bc79936) to head (7ec5478).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #493      +/-   ##
==========================================
+ Coverage   91.22%   94.25%   +3.02%     
==========================================
  Files           5        6       +1     
  Lines          57       87      +30     
  Branches       19       23       +4     
==========================================
+ Hits           52       82      +30     
  Misses          5        5

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

dcoa · 2025-01-27T23:11:53Z

Hi, @brian-smith-tcril I has wondering if you can have a look to this proposal or if should I tag someone from product before the technical review?

brian-smith-tcril · 2025-01-28T03:38:48Z

Sorry for the extended delay in response!

I think this is a change we do want. I also know that there is a similar feature implemented in frontend-base at the moment. Ideally the implementation of this feature will align with what is needed by the feature in frontend-base. I think @arbrandes has a better sense of what the frontend-base implementation is at the moment, and what we'll want it to be by the time we are ready to start using frontend-base.

Assuming this PR and the implementation in frontend-base play well together I'd be more than happy to review it!

sarina · 2025-01-28T18:54:14Z

should I tag someone from product before the technical review

I think there's a question of whether or not we want to be adding a new flag. If we don't mind a new flag this PR is good to go from a product perspective, but if we want to make this the default experience for any site that has multiple languages, you'd want to make a product ticket.

dcoa · 2025-01-31T00:48:11Z

@arbrandes I think your opinion here is valuable, understanding you have a better panorama of the current state of the frontend new architecture.

What do you think about this proposal and the alignment with frontend-base also the introduction of new config variables?

mphilbrick211 · 2025-02-12T22:07:10Z

@arbrandes I think your opinion here is valuable, understanding you have a better panorama of the current state of the frontend new architecture.

What do you think about this proposal and the alignment with frontend-base also the introduction of new config variables?

Friendly ping on this, @arbrandes

dcoa · 2025-03-31T20:55:19Z

@arbrandes any opinion in this topic?

Ang-m4 · 2025-04-01T13:43:12Z

Hi @dcoa,
I hope you're doing well!
I came across this PR while working with the NELC team, as we were looking for a simple and practical implementation of a language selector. We really appreciate your proposal and are excited about the possibility of using it. We're also looking forward to seeing it merged upstream.
Additionally, we're currently working on a header implementation that follows the same logic. I’d appreciate it if you could take a look at it soon.

I’ll be leaving some comments on your code for further discussion, and i really would like to @arbrandes to take a look at this as he has a better understanding of the entire project.

Thank you for your hard work and contributions!

Ang-m4 · 2025-04-01T13:55:07Z

src/components/Footer.jsx

+          {config.ENABLE_FOOTER_LANG_SELECTOR && (
+            <div className="mb-2">
+              <LanguageSelector
+                options={config.SITE_SUPPORTED_LANGUAGES}
+                username={authenticatedUser?.username}
+                langCookieName={config.LANGUAGE_PREFERENCE_COOKIE_NAME}
+              />
+            </div>


I think it might be cleaner to handle some rendering logic validation inside the component, along with fetching the options, username, and langCookieName there.

Ang-m4 · 2025-04-01T14:00:17Z

src/components/LanguageSelector/index.jsx

+};
+const getLocaleName = (locale) => {
+  const langName = new Intl.DisplayNames([locale], { type: 'language', languageDisplay: 'standard' }).of(locale);
+  return langName.replace(/^\w/, (c) => c.toUpperCase());


Suggested change

return langName.replace(/^\w/, (c) => c.toUpperCase());

return langName.charAt(0).toUpperCase() + langName.slice(1);

I see that this line capitalizes the string. A more straightforward implementation without regular expressions might be better for this task. I'll leave it up to you.

Ang-m4 · 2025-04-01T14:01:56Z

src/components/LanguageSelector/index.test.jsx

+    });
+  });
+
+  it('should disp,ay the language icon and modify the label according to the screen size', () => {


Suggested change

it('should disp,ay the language icon and modify the label according to the screen size', () => {

it('should display the language icon and modify the label according to the screen size', () => {

Ang-m4 · 2025-04-01T14:08:19Z

src/components/LanguageSelector/index.jsx

I suggest adding docstrings to this file methods to improve readability.

Ang-m4 · 2025-04-01T14:09:04Z

src/components/LanguageSelector/data.js

I suggest adding docstrings to this file methods to improve readability.

I would like you to take a look at this PR, you might find it interesting as it proposes a new frontend-platform module to handle the language change logic based on your changes.

Ang-m4 · 2025-04-07T15:42:18Z

Hi @dcoa,

I've tested the final version of the component using frontend-app-learning and it looks great visually! However, I noticed an issue: when a new language is selected from the dropdown, it only updates the cookie in LMS, which prevents the application from loading and applying the correct translations. This happens because the getLocale() method (also used by handleRtl()) relies on the LANGUAGE_PREFERENCE cookie to retrieve its information. I recommend moving the cookie assignment outside of the conditional statement.

Please see the attached video demonstrating the behavior:

translations-not-working.mp4

After the change, everything seems to work fine:

translations-working.mp4

However, please note that while the translations do update, they are only applied partially. Many sections of the platform are still not configured to receive the publish() event or handle translations dynamically.

Given this, the most robust solution might be to force a full page reload when updating the language, ensuring that all parts of the application reflect the new language settings.

What do you think?

Ang-m4 · 2025-04-07T15:44:12Z

src/components/LanguageSelector/index.jsx

+  try {
+    if (username) {
+      await patchPreferences(username, { prefLang: selectedLlocale });
+      await postSetLang(selectedLlocale);
+    } else {
+      getCookies().set(langCookieName, selectedLlocale);
+    }


Suggested change

try {

if (username) {

await patchPreferences(username, { prefLang: selectedLlocale });

await postSetLang(selectedLlocale);

} else {

getCookies().set(langCookieName, selectedLlocale);

}

try {

getCookies().set(langCookieName, selectedLlocale);

if (username) {

await patchPreferences(username, { prefLang: selectedLlocale });

await postSetLang(selectedLlocale);

}

I thing the real problem is there are 2 cookies one is being updated the second one not, I will check if I can replicate the behavior and understand why that is happening. In any case, your suggest is good.

The suggested change creates a new cookie instead of changing the default one, because of that is not the best solution for the described problem.

I could identify that the described problem is only present in learning (that make sense for the iframe - django based does not change at runtime as react does).

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jan 13, 2025

dcoa changed the title ~~Dcoa/lang selector~~ refactor: change lang selector logic Jan 13, 2025

dcoa force-pushed the dcoa/lang-selector branch 8 times, most recently from 0ae49f6 to 3845c7f Compare January 13, 2025 09:37

dcoa marked this pull request as ready for review January 13, 2025 09:40

dcoa requested a review from arbrandes January 13, 2025 10:16

dcoa force-pushed the dcoa/lang-selector branch from 3845c7f to bd327ad Compare January 21, 2025 04:50

dcoa added 2 commits January 28, 2025 08:19

refactor: change lang selector logic

1dd8994

refactor: separate the test for lang seector

7ec5478

dcoa force-pushed the dcoa/lang-selector branch from bd327ad to 7ec5478 Compare January 27, 2025 21:19

Ang-m4 reviewed Apr 1, 2025

View reviewed changes

Ang-m4 mentioned this pull request Apr 3, 2025

feat: add language selector component to header openedx/frontend-component-header#586

Open

Ang-m4 reviewed Apr 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: change lang selector logic #493

refactor: change lang selector logic #493

dcoa commented Jan 13, 2025 •

edited

Loading

openedx-webhooks commented Jan 13, 2025 •

edited

Loading

codecov bot commented Jan 13, 2025 •

edited

Loading

dcoa commented Jan 27, 2025

brian-smith-tcril commented Jan 28, 2025

sarina commented Jan 28, 2025

dcoa commented Jan 31, 2025

mphilbrick211 commented Feb 12, 2025

dcoa commented Mar 31, 2025

Ang-m4 commented Apr 1, 2025

Ang-m4 Apr 1, 2025 •

edited

Loading

Ang-m4 Apr 1, 2025

Ang-m4 Apr 1, 2025

Ang-m4 Apr 1, 2025

Ang-m4 Apr 1, 2025

Ang-m4 Apr 3, 2025

Ang-m4 commented Apr 7, 2025

Ang-m4 Apr 7, 2025

dcoa Apr 23, 2025 •

edited

Loading

dcoa May 5, 2025

	return langName.replace(/^\w/, (c) => c.toUpperCase());
	return langName.charAt(0).toUpperCase() + langName.slice(1);

	it('should disp,ay the language icon and modify the label according to the screen size', () => {
	it('should display the language icon and modify the label according to the screen size', () => {

refactor: change lang selector logic #493

Are you sure you want to change the base?

refactor: change lang selector logic #493

Conversation

dcoa commented Jan 13, 2025 • edited Loading

Description and motivation

Proposal

How to test

openedx-webhooks commented Jan 13, 2025 • edited Loading

codecov bot commented Jan 13, 2025 • edited Loading

Codecov Report

dcoa commented Jan 27, 2025

brian-smith-tcril commented Jan 28, 2025

sarina commented Jan 28, 2025

dcoa commented Jan 31, 2025

mphilbrick211 commented Feb 12, 2025

dcoa commented Mar 31, 2025

Ang-m4 commented Apr 1, 2025

Ang-m4 Apr 1, 2025 • edited Loading

Choose a reason for hiding this comment

Ang-m4 Apr 1, 2025

Choose a reason for hiding this comment

Ang-m4 Apr 1, 2025

Choose a reason for hiding this comment

Ang-m4 Apr 1, 2025

Choose a reason for hiding this comment

Ang-m4 Apr 1, 2025

Choose a reason for hiding this comment

Ang-m4 Apr 3, 2025

Choose a reason for hiding this comment

Ang-m4 commented Apr 7, 2025

Ang-m4 Apr 7, 2025

Choose a reason for hiding this comment

dcoa Apr 23, 2025 • edited Loading

Choose a reason for hiding this comment

dcoa May 5, 2025

Choose a reason for hiding this comment

dcoa commented Jan 13, 2025 •

edited

Loading

openedx-webhooks commented Jan 13, 2025 •

edited

Loading

codecov bot commented Jan 13, 2025 •

edited

Loading

Ang-m4 Apr 1, 2025 •

edited

Loading

dcoa Apr 23, 2025 •

edited

Loading