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

ICU: Fixes macro to support count prop and expressions better #939

Merged
merged 18 commits into from
Sep 11, 2019

Conversation

redbugz
Copy link
Contributor

@redbugz redbugz commented Sep 10, 2019

Fixes for #937
Allows count prop to use a variable also named count
Add support for expressions in the count and switch props
Handles the values prop better
Allows macros to be self-closing without children
Trims newlines and whitespace around code formatted with prettier or other formatting tools

If the count prop is also named count, it copies it to the count prop of Trans:

<Plural count={count} ---> <Trans count={count}

If count is an expression it also copies it to Trans as the count prop:

<Plural count={index+1} ---> <Trans count={index+1}

Also, if code is formatted with prettier, the extra whitespace was getting into the translations, so now trims the whitespace back to normal

If you had a values prop and a count, sometimes they didn't merge correctly. I believe that is fixed now.

<Plural
  count={number}
  values={{ prop1: value1, prop2: value2 }}
/>
// this transforms to:
<Trans values={{ number, prop1: value1, prop2: value2 }} />

The above also works for SelectOrdinal and Select with the switch prop.

redbugz and others added 18 commits August 26, 2019 04:23
Fix for i18next/i18next#1295
Adds support for ICU message syntax inside the `defaults` attribute
This allows the Trans macro to be used with linters and other code analysis
tools that expect valid JSX syntax as children, by using the defaults
prop instead of children for the message.

Also adds support fo the `<SelectOrdinal>` tag
Fixes a bug where `Trans` was imported from 'react/i18next' instead of 'react-i18next'
In the `useTranslation` hook, `useContext` is called conditionally, which breaks the Rule of Hooks, so it may cause problems in the future. This fixes it by calling `useContext` on every render, just not always using the result from it.
Fixes for i18next#937
Allows count prop to use a variable also named count
Add support for expressions in the count and switch props
Handles the values prop better
Allows macros to be self-closing without children
Trims newlines and whitespace around code formatted with prettier or other formatting tools
@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 95.377% when pulling 5b97c81 on redbugz:fix-count into db6e7d0 on i18next:master.

@redbugz
Copy link
Contributor Author

redbugz commented Sep 10, 2019

Somehow in my merge/rebase of master, I ended up with lots of extra commits, but the changed files is correct. If you would like me to clean up the commits, I can do the PR again.

// take the switch for select element
let exprName = attr.node.value.expression.name;
if (!exprName) {
exprName = 'selectKey';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't use switch because it is a reserved word. I'm open to alternate names for this.

It is used when switch is an expression.

<Select
  switch={'fe'+'male'} // very contrived example
  ...
/>
// This transforms to:
<Trans values={{ selectKey: 'fe'+'male' }} defaults="{selectKey, select, ....} />

I don't think this will be used very often, but it was the same logic as Plural and count so I included it for completeness.

Copy link
Member

Choose a reason for hiding this comment

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

looks ok for me

const thisTrans = processTrans(children, babel, componentStartIndex);

let pluralForm = attr.node.name.name;
if (pluralForm.indexOf('$') === 0) pluralForm = pluralForm.replace('$', '=');

mem.defaults = `${mem.defaults} ${pluralForm} {${thisTrans.defaults}}`;
mem.components = mem.components.concat(thisTrans.components);
mem.values = mem.values.concat(thisTrans.values);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was causing problems with duplicate keys in values, and I'm not sure I understand the use case and all the test cases I needed are passing, so I just removed it.
If we need to support this, then I can add code to dedupe these from values.

const thisTrans = processTrans(children, babel, componentStartIndex);

mem.defaults = `${mem.defaults} ${attr.node.name.name} {${thisTrans.defaults}}`;
mem.components = mem.components.concat(thisTrans.components);
mem.values = mem.values.concat(thisTrans.values);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

see comment above for similar code

@jamuhl
Copy link
Member

jamuhl commented Sep 11, 2019

Will check this later today and merge/publish

@jamuhl jamuhl merged commit 84f308d into i18next:master Sep 11, 2019
@jamuhl
Copy link
Member

jamuhl commented Sep 11, 2019

published in [email protected]

@redbugz redbugz deleted the fix-count branch September 12, 2019 00:42
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.

5 participants