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

Support react-i18n <Trans> components #44

Merged
merged 4 commits into from Sep 16, 2017
Merged

Support react-i18n <Trans> components #44

merged 4 commits into from Sep 16, 2017

Conversation

wichert
Copy link
Contributor

@wichert wichert commented Sep 12, 2017

This adds basic support for use of Trans components. There are probably still a few missing pieces, but it seems to work for me.

This adds basic support for use of Trans components. There are probably
still a few missing pieces, but it seems to work for me.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.5%) to 92.537% when pulling b54b7db on curvetips:trans-component into 1d5c50e on i18next:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.5%) to 92.537% when pulling b54b7db on curvetips:trans-component into 1d5c50e on i18next:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.5%) to 92.537% when pulling b54b7db on curvetips:trans-component into 1d5c50e on i18next:master.

@wichert
Copy link
Contributor Author

wichert commented Sep 12, 2017

There are a few limitations in this code:

  • nested tags do not work correctly. react-i18n seems to number inside out, while this code is numbering outside-in. For example You have <a>a <i>very</I> bad</a> apple is converted to You have <1>a <2>very</2> bad</1> apple instead of You have a <1>a <0>very</0> bad</1>apple.
  • {...} expressions are not handled correctly. This should transform You have {{count}} apples to You have <1>{{count}}</1> apples.

I don't know what the exact numbering rules for Trans are, so I am not sure if it makes sense to try and fix things here with trial and error. Fixing up the {...} expressions should not be very hard, but I do not know if Trans makes any assumptions about tag numbering between elements and expressions.

@jamuhl
Copy link
Member

jamuhl commented Sep 12, 2017

numbering rules are based on the index of the node: https://github.com/i18next/react-i18next/blob/master/src/trans.js#L15

@wichert
Copy link
Contributor Author

wichert commented Sep 12, 2017

Hm, so to fully support it most likely I will need to add a HTML parser that can also deal with {...} expressions to create an AST, and walk through that in depth-first order get everything correct. That will make for a much larger PR and probably add a dependency on a parser or parser generator. Is that something you would be willing to accept ?

@coveralls
Copy link

coveralls commented Sep 14, 2017

Coverage Status

Coverage increased (+1.05%) to 93.113% when pulling 5a214fe on curvetips:trans-component into 1d5c50e on i18next:master.

@wichert
Copy link
Contributor Author

wichert commented Sep 14, 2017

I've pushed a reimplementation which switches to using parse5 to parse the fragment to build an AST, and then use that to generate output. The result should be completely accurate generating of text.

A possible next step might be to convert parseAttrFromString to work from the same AST as well so it doesn't have to try and parse HTML using regular expressions. That should simplify the code quite a bit as well. Let me know if you want me to add that.

// Parses translation keys from `Trans` components in JSX
// <Trans i18nKey="some.key">Default text</Trans>
parseTransFromString(content, opts = {}, customHandler = null) {
const pattern = '<Trans[^]*?i18nKey="([^"]+)"[^]*?>([^]*?)</\\s*Trans\\s*>';
Copy link
Member

Choose a reason for hiding this comment

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

@wichert Is it possible to make Trans and i18nKey configurable?

@cheton
Copy link
Member

cheton commented Sep 14, 2017

I strongly agree with your suggestion to convert parseAttrFromString to use AST instead regular expressions., it will be great if you can add AST support to it.

Thank you for your contribution to make it happen.

@wichert
Copy link
Contributor Author

wichert commented Sep 15, 2017

@cheton Done!

I could make Trans and i18nKey configurable, but I'm not entirely sure what the use case for that is so I'm a bit hesitant to complicate the code. Are you thinking of other i18next wrappers that use different elements? I'm not sure what the integrations with AngularJS, Vue or Aurelia look like from memory. I'ld like to know that so I can make a better decision about the API; in particular if the values should be strings or regular expressions.

@coveralls
Copy link

coveralls commented Sep 15, 2017

Coverage Status

Coverage increased (+0.6%) to 92.683% when pulling 653cf0f on curvetips:trans-component into 1d5c50e on i18next:master.

@cheton cheton merged commit a7f31e0 into i18next:master Sep 16, 2017
@wichert wichert deleted the trans-component branch November 6, 2017 20:31
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.

None yet

4 participants