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

feat(eslint): add i18n eslint plugin #362

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

feat(eslint): add i18n eslint plugin #362

wants to merge 2 commits into from

Conversation

ismay
Copy link
Member

@ismay ismay commented Mar 10, 2021

See this slack thread: https://dhis2.slack.com/archives/CPGUFVC56/p1615363540343500

This adds https://github.com/edvardchen/eslint-plugin-i18next, which warns on untranslated strings in jsx. The settings are what I've been using in the scheduler. We could tweak them a bit more later on, and for example add linting of certain attributes as well. I'm ignoring this rule for test files, as react components defined in test files won't be shipped to the end user.

I've set it to warn initially so that it's not a breaking change and to allow us to try it out first. Once we're happy with it we could move it to error and fine-tune the rule.

Copy link
Contributor

@HendrikThePendric HendrikThePendric left a comment

Choose a reason for hiding this comment

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

Adding it like this would be fine with me, but we could also look into perhaps declaring a few attribute names where we do want to enforce i18n.

@@ -14,6 +16,10 @@ module.exports = {
},

rules: {
'i18next/no-literal-string': [
'warn',
{ markupOnly: true, onlyAttribute: [''] },
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we could change this so that label and displayName attributes would require i18n?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I could do that. Is it necessary to translate displayName though? That's only for developers right?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've enabled it for label in 4e5acac. I've not enabled it for displayName as that's only used in error messages and not user facing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Enabling it for label has the expected result, so seems like a good addition:

Screenshot 2021-03-10 at 11 21 28

@varl

This comment has been minimized.

@stale
Copy link

stale bot commented Mar 2, 2022

Hi!

Due to a lack of activity on this issue over time (180 days) it seems to be stale. If still relevant, please provide information that moves it forward, e.g. additional information, a pull request with suggested changes, or a reason to keep it open.

Any activity will keep it open, otherwise it will be closed automatically in 30 days. Thanks! 🤖

@stale stale bot added the stale label Mar 2, 2022
@stale
Copy link

stale bot commented Apr 16, 2022

Automatically closing due to lack of activity. 🤖

@stale stale bot closed this Apr 16, 2022
@ismay ismay reopened this Oct 11, 2022
@ismay ismay removed the request for review from varl October 11, 2022 08:06
@amcgee amcgee removed their request for review April 25, 2024 09:06
@stale stale bot removed the stale label Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants