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

Allow non-list children to render correctly with i18nIsDynamicList #1658

Closed
nicegamer7 opened this issue Aug 3, 2023 · 1 comment · Fixed by #1660
Closed

Allow non-list children to render correctly with i18nIsDynamicList #1658

nicegamer7 opened this issue Aug 3, 2023 · 1 comment · Fixed by #1660

Comments

@nicegamer7
Copy link
Contributor

🚀 Feature Proposal / Possible bug fix

Hi there!

Proposals (all of which I can open a PRs for, if desired):

  1. Allow i18nIsDynamicList to function correctly, even when child is not a list.
  2. Rename i18nIsDynamicList to i18nIsDynamicContent (will be done in a backwards compatible way, i.e. both will exist for a time, maybe till v14)

Motivation

  1. I believe this first proposal is actually a bugfix. Currently, the Trans component assumes that the child of an element with the i18nIsDynamicList prop is a list, which may not be the case. See the following example:
const someDynamicVar = cond ? <Comp /> : 'Text';

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

The above will not render correctly. However, the following does:

const someDynamicVar = cond ? <Comp /> : 'Text';

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

By adding a fragment, the child becomes a list, and therefore the content renders correctly. My proposal is to make it work without the need for a list.

  1. If we make the above change, which seems to me to be a bugfix, it would make sense to rename the i18nIsDynamicList prop to something that better describes what it's for. I was thinking i18nHasDynamicChildren. We can make this change in a backwards compatible way (i.e. both i18nIsDynamicList and i18nHasDynamicChildren will exist for a time, maybe till v14)

Please let me know if these changes are welcome, in which case I'll make a PR!

@adrai
Copy link
Member

adrai commented Aug 4, 2023

Feel free to create a PR to address this, but I would not rename the prop.

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 a pull request may close this issue.

2 participants