-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
basic i18n support #5153
basic i18n support #5153
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #5153 +/- ##
==========================================
- Coverage 86.94% 86.68% -0.26%
==========================================
Files 560 560
Lines 44220 44223 +3
Branches 6854 6853 -1
==========================================
- Hits 38446 38336 -110
- Misses 5774 5887 +113
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
Makefile.dryice.js
Outdated
allMessages[eng] = ""; | ||
}); | ||
}); | ||
fs.writeFileSync(__dirname + "/messages.json", JSON.stringify(allMessages, null, 4), "utf8"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if this throws error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it will be printed in the terminal, and the developer running it will decide what to do.
Are we going to update all aria-labels like texts to the nls versions in a separate PR? |
6b3e547
to
5e4bcb9
Compare
Good point, i have forgotten to check aria-labels, updated now. |
705eddf
to
fd62093
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than documentation LGTM. I think we can also update strings which need translations in upcoming PRs if we see the need.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I've missed Alice's comment: #5153 (comment)
Let's update those aria labels as well.
This has broken the default example of While that is beyond the purview of this repo I am curious what we can do to fix it. |
@FirstWhack could you please doublecheck that ace.js and ext-searchbox.js loaded in your page are from the same version of ace-builds? I couldn't reproduce the issue you describe by running git clone https://github.com/securingsincity/react-ace
cd react-ace
npm i
npm i ace-builds@latest fixing webpack.config.example.js // contentBase: [
// path.join(__dirname, "example"),
// path.join(__dirname, "dist")
// ],
static: {
directory: path.join(__dirname, './example'),
serveIndex: true,
}, running npm run example and in browser console [ace.version, ace.config.nls] prints but the error you describe would happen if another version of ace.js was included in the page. |
I suppose that is a better way to describe the issue. React-ace has not been updated in around a year and is pointed towards ace-builds ^1.4.14 returning ace.js version 1.16. If I use yarn resolutions to force latest it works fine. Definitely not an issue with ace itself. |
Issue #2118
Adds a global option for translation messages. Already rendered messages are not changed when the option is changed.
Another limitation is that this does not support plurals for now, in the future if we decide to support it we can change nls to
which could be called as