Fix unescape logic, expression props, and ICU format for Trans component #892
Conversation
nicegamer7
changed the title
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #892 +/- ##
==========================================
+ Coverage 93.95% 93.97% +0.01%
==========================================
Files 11 11
Lines 1836 1841 +5
==========================================
+ Hits 1725 1730 +5
Misses 111 111
☔ View full report in Codecov by Sentry. |
|
Actually, I'm going to put this on hold until i18next/react-i18next#1663 is resolved, since it might mean we don't have to revert #871. edit: i18next/react-i18next#1663 was fixed, so there's no need to revert #871 anymore. |
nicegamer7
force-pushed
the
hotfix/unescape-key
branch
from
16a90ee
to
905082a
Compare
nicegamer7
changed the title
| assert.equal(Lexer.extract(content)[0].key, "I'm Cielquan") | ||
| done() | ||
| }) | ||
|
|
||
| it('supports the shouldUnescape options', (done) => { |
There was a problem hiding this comment.
I find this a little counter intuitive and confusing. I would expect that shouldUnescape would unescape the defaultValue. If I understand correctly, that's not the case in react-18next, is that right?
Also, I'm wondering if we should add a test with a i18nKey property with escaped value. wdyt?
There was a problem hiding this comment.
It is a little confusing, but I'll try to explain as I understand it.
The shouldUnescape option means that after the translation is loaded/displayed, it will be unescaped. That's why when we see the shouldUnescape option, we don't unescape when extracting, so that it can be unescaped by react-i18next when it is loaded/displayed.
Also, I think it makes sense to add a test for a i18nKey with an escaped value, nice suggestion.
nicegamer7
changed the title
|
Hey @karellm, I just pushed another commit that fixes two more issues that I noticed. The two issues are:
Take a look when you have a chance and let me know if there are any concerns! |
|
LGTM! Thanks for the great work and it is now deployed as |
Why am I submitting this PR
After #871, it seems I introduced a regression, though it did help find a bug.
It seems that react-i18next does indeed default the key to theAfter reporting this on react-i18next's bug tracker (i18next/react-i18next#1663), it looks like this was unintended. I fixed that, so no need to revert #871.defaultsprop (I added a comment to prevent anyone from making the same mistake again). I've reverted the changes from #871 to match react-i18next's behavior.Otherwise, it seems we had a bug where we were unescaping values in reverse (i.e. unescaping when it wasn't needed, and not unescaping when it was needed). I corrected this, and also made it so that we always unescape the key, since this matches react-i18next's behavior as well.
Does it fix an existing ticket?
Closes #886.
Checklist
yarn test(see details here)