Skip to content

MCKIN-21527 Problem Builder (FTE, Assessment, MCQ, MRQ) - On opening these modules, none of the text is translated on notifications dropdown. - #294

Closed
wasifarbisoft wants to merge 4 commits into
open-craft:problem-builder-v3from
wasifarbisoft:MCKIN-21527-p1
Closed

MCKIN-21527 Problem Builder (FTE, Assessment, MCQ, MRQ) - On opening these modules, none of the text is translated on notifications dropdown.#294
wasifarbisoft wants to merge 4 commits into
open-craft:problem-builder-v3from
wasifarbisoft:MCKIN-21527-p1

Conversation

@wasifarbisoft

@wasifarbisoft wasifarbisoft commented Aug 19, 2020

Copy link
Copy Markdown
Contributor

Issue:
Problem Builder's gettext and ngettext override the global translations of the edx app. Hence strings of apps added outside the scope of problem builder doesn't get translated specially edx-notification, this is due to the step_util.js and util.js in which gettext and ngetext are declared globally.

Solution:
New variables are introduced for the problem builder i.e pb_gettext and pb_ngettext so that global gettext and ngettext don't get overridden and their usage is updated elsewhere.

…these modules, none of the text is translated on notifications dropdown.
@wasifarbisoft

Copy link
Copy Markdown
Contributor Author

@xitij2000 Can you please review this pull request?

@xitij2000

Copy link
Copy Markdown
Member

@wasifarbisoft I still see a couple of uses of the regular gettext in instructor_tool.js and questionnaire.js

Additionally, with this change there is a lot of dead code introduced. For instance, a lot of files include the following snippet:

// Set up gettext in case it isn't available in the client runtime:
    if (typeof gettext == "undefined") {
        window.gettext = function gettext_stub(string) { return string; };
        window.ngettext = function ngettext_stub(strA, strB, n) { return n == 1 ? strA : strB; };
    }

This is no longer needed as we're not using window.gettext anymore, so it can be removed.

@wasifarbisoft

Copy link
Copy Markdown
Contributor Author

@xitij2000
I have updated the Pull Request. Please review it.

@xitij2000

Copy link
Copy Markdown
Member

@wasifarbisoft I just tested this from scratch, and I don't think this approach will work. While it does solve the problem from browser side, it breaks extracting translations. I think the best approach here is to make the gettext and ngettext declaraton from being global to local to the function, instead of renaming them everywhere, which would mean that translations can no longer be extracted because the tooling searches for gettext not pb_gettext.

@wasifarbisoft

Copy link
Copy Markdown
Contributor Author

Thanks @xitij2000 ,

I am closing this Pull Request and based on your last comments I have created a new Pull Request
#298

Please review that thanks!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants