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

Fix non-list dynamic content in Trans component #1660

Merged
merged 5 commits into from
Aug 10, 2023

Conversation

nicegamer7
Copy link
Contributor

@nicegamer7 nicegamer7 commented Aug 8, 2023

Hi! This PR closes #1658.

Changes

  • Always return children as an array from getChildren (for isI18nDynamicList)
    • This is the bugfix
  • Reduce false positives when checking for nodes needing handling with empty children
    • This is just something I noticed while working
  • Remove React warnings in Trans component by removing i18nIsDynamicList prop while rendering
    • Again, this is just something I noticed and thought to fix while I was at it

Checklist

  • only relevant code is changed (make a diff before you submit the PR)
  • run tests yarn test
  • tests are included
  • commit message and code follows the Developer's Certification of Origin

@coveralls
Copy link

coveralls commented Aug 8, 2023

Coverage Status

coverage: 96.167% (+0.2%) from 95.98% when pulling 5e34bdd on nicegamer7:hotfix/dynamic-content into f8ea7d8 on i18next:master.

@adrai adrai requested review from jamuhl and pedrodurek August 8, 2023 18:40
index.d.ts Show resolved Hide resolved
@jamuhl
Copy link
Member

jamuhl commented Aug 9, 2023

What is the use case of having i18nIsDynamicList in combination with a non-list?

You give the sample:

<Trans>
  Hi there,{' '}
  <React.Fragment i18nIsDynamicList>
    {someDynamicVar}
  </React.Fragment>
</Trans>

which has nothing todo with the usage of i18nIsDynamicList

that flag is only used for creating the string for saveMissing feature and only for the case where content is like: https://react.i18next.com/latest/trans-component#using-with-lists-v10.5.0

@nicegamer7
Copy link
Contributor Author

In our project we're using key fallback for all translations, so the key for <Trans>Hi</Trans> would be "Hi". This worked well for 99% of our project, but in a couple places, we had dynamic variables being used, and these weren't being extracted correctly by i18next-parser (rather than extracting as <3></3> for example, it extracted as {someDynamicVar}). This caused the translations to not get matched properly to their respective Trans elements, so I thought this was the expected behaviour since it worked with fine with i18nIsDynamicList (after this bug fix, and another to i18next-parser).

It seems extracting JSX expressions is broken in i18next-parser, so I'll get that fixed in another PR. Regardless though, there is still a valid use for this PR which I came across in another file yesterday:

<Trans>
  <React.Fragment isI18nDynamicList>
    {list.length === 1 ? list[0] : list.map((x) => <div> ... </div>)}
  </React.Fragment>
</Trans>

We have a couple different pieces of code that are similar to the above in our project, where we want to display the list differently depending on whether it has 1 element, or multiple. We could split it into a ternary with two Trans elements, but this reduces duplicate code.

src/TransWithoutContext.js Outdated Show resolved Hide resolved
@adrai adrai merged commit a270c35 into i18next:master Aug 10, 2023
@adrai
Copy link
Member

adrai commented Aug 10, 2023

@nicegamer7 shouldn't that warning be gone?
image
// cc: @pedrodurek

@adrai
Copy link
Member

adrai commented Aug 10, 2023

btw: included in v13.1.0

@nicegamer7
Copy link
Contributor Author

@adrai I wasn't able to remove the warning from React.Fragment, since it has its own validation that happens before rendering occurs. But the warning is removed for all HTML elements.

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.

Allow non-list children to render correctly with i18nIsDynamicList
4 participants