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.

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

Remove all instances of the crossComponentTranslator before the next string freeze #9037

Closed
MisRob opened this issue Jan 25, 2022 · 9 comments · Fixed by #10145
Closed

Remove all instances of the crossComponentTranslator before the next string freeze #9037

MisRob opened this issue Jan 25, 2022 · 9 comments · Fixed by #10145
Assignees
Labels
DEV: frontend good first issue Self-contained, straightforward, low-complexity i18n Issues with fonts and translations in localizations P1 - important Priority: High impact on UX TAG: tech update / debt Change not visible to user TAG: user strings Application text that gets translated

Comments

@MisRob
Copy link
Contributor

MisRob commented Jan 25, 2022

Observed behavior

After the string freeze for the 0.15 release, we couldn't modify strings and their namespaces anymore. That also means that we couldn't move strings between components. We have the crossComponentTranslator utility which allowed us to access translation strings in a component from another component so that we could temporarily work around this. Before the next string freeze, we should clean up all places where the crossComponentTranslator is used, move strings to components from where they are really used or to common string files, and remove all components that are not used but needed to be left in the codebase because they contain some translations.

Example

Translations strings from the OverallStatus component

$trs: {
// eslint-disable-next-line kolibri/vue-no-unused-translations
goal: {
message: 'Get {count, number, integer} {count, plural, other {correct}}',
context:
'Message that indicates to the learner how many correct answers they need to give in order to master the given topic, and for the exercise to be considered completed.',
},
},

should be moved to the LessonMasteryBar component

and used directly instead of using the crossComponentTranslator

const overallStatusStrings = crossComponentTranslator(OverallStatus);

:text="overallStatusStrings.$tr('goal', { count: requiredCorrectAnswers })"

OverallStatus component should then be removed.

Note that removing a component might not be desired at all places that need to be cleaned up since sometimes we share strings between components that are all used. In such cases, the crossComponentTranslator should still be removed and strings can be duplicated in both components, alternatively, they could be moved to shared string files like for example commonCoreStrings, commonLearnStrings, etc. depending on what location is appropriate.

Search codebase for the crossComponentTranslator term to find more.

@MisRob MisRob added this to the 0.16.0 milestone Jan 25, 2022
@MisRob MisRob added DEV: frontend TAG: tech update / debt Change not visible to user i18n Issues with fonts and translations in localizations P2 - normal Priority: Nice to have good first issue Self-contained, straightforward, low-complexity TAG: user strings Application text that gets translated labels Jan 25, 2022
@marcellamaki marcellamaki added P1 - important Priority: High impact on UX and removed P2 - normal Priority: Nice to have labels Jul 6, 2022
@photon0205
Copy link
Contributor

Hi!!, Is this issue still valid?

@MisRob
Copy link
Contributor Author

MisRob commented Jan 18, 2023

Hello @photon0205, yes it is still valid, are you asking because you'd like to work on it?

@photon0205
Copy link
Contributor

Yes, I would like to work on this

@MisRob
Copy link
Contributor Author

MisRob commented Jan 18, 2023

Thank you, we appreciate that. I'll assign you. Feel free to start and let me know if you encountered any issues. Depending on how many instances of crossComponentTranslator are currently in the codebase, you may open one pull request or split work into more pull requests if that'd be easier.

@sahej-hira
Copy link

I would like to would on this issue. If its still valid, please assign it to me.

@photon0205
Copy link
Contributor

@shark-lamp I am working on it

@radinamatic
Copy link
Member

radinamatic commented Jan 27, 2023

the crossComponentTranslator should still be removed and strings can be duplicated in both components, alternatively, they could be moved to shared string files like for example commonCoreStrings, commonLearnStrings, etc. depending on what location is appropriate.

I did not review the code, but noting that moving the identical strings used in different components into commonCoreStrings or commonLearnStrings is almost always the preferred solution, as duplicating them just inflates our translation word count and needlessly increases the i18n budget.

@marcellamaki marcellamaki self-assigned this Feb 17, 2023
@Akila-I
Copy link
Contributor

Akila-I commented Feb 22, 2023

Hi @MisRob @marcellamaki ,
Is this issue still open? If so, I would love to take and work on it.
I have done working on the issue #9867 and the PR #10130 is about to get merged. So I would be free to work on another issue.

@MisRob
Copy link
Contributor Author

MisRob commented Feb 23, 2023

Thank you, @Akila-I, we appreciate your interest. This particular issue is currently time-sensitive so it's been already been assigned to @marcellamaki, who will start working on it soon. I saw that you've already posted a comment of interest to one another issue, so I'd recommend focusing on that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DEV: frontend good first issue Self-contained, straightforward, low-complexity i18n Issues with fonts and translations in localizations P1 - important Priority: High impact on UX TAG: tech update / debt Change not visible to user TAG: user strings Application text that gets translated
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants