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

Add TransText component to allow Trans behaviour for strings #641

Merged
merged 4 commits into from
Nov 17, 2021

Conversation

dndhm
Copy link
Contributor

@dndhm dndhm commented Jul 1, 2021

Because Trans handles useTranslation internally, it means it cannot easily be used in conjunction with returnObjects when mapping over arrays of values containing components:

{
  "content-list": [
    "List of <0>things</0>",
    "with <1>variously</1> nested",
    "components and <2><1>content</1></2>!"
  ]
}
const contentList = t('someNamespace:content-list', {}, { returnObjects: true });

Rather than extending and overloading Trans, I am using a simple text-only version of it in a project locally, which I feel could be usefully added to the core package:

{contentList.map((listItem: string) => (
  <TransText
    text={listItem}
    components={[
      <a href="some-url" />,
      <b />,
      <em />,
    ]}
  />
)}

The functionality of Trans remains unchanged. I have hoisted formatElements into a separate file so it can be used by both Trans and TransText.

@AndrewBestbier
Copy link

Would love to have this in!

@dndhm
Copy link
Contributor Author

dndhm commented Aug 9, 2021

Thanks @AndrewBestbier!

Do any maintainers have an opinion on this?

@aralroca aralroca added this to the 1.1.0 milestone Aug 22, 2021
@aralroca
Copy link
Owner

Related with this discussion: #626

Sorry for taking so long, I had an accident and have been in the hospital for a while. Now that I'm back, I'll take a good look at the PR during this week. I think it's a good idea, but I don't know if there was a more practical solution, I'll check it out.

@dndhm
Copy link
Contributor Author

dndhm commented Aug 22, 2021

No problem - sorry to hear about your accident!

@aralroca aralroca modified the milestones: 1.1.0, future Sep 18, 2021
@aralroca aralroca deleted the branch aralroca:canary November 7, 2021 15:07
@aralroca aralroca closed this Nov 7, 2021
@aralroca aralroca reopened this Nov 7, 2021
@aralroca
Copy link
Owner

aralroca commented Nov 7, 2021

Sorry, I removed the canary branch when I made the release by accident and the PRs that were open have been closed. I am committed to review the PR during this week. Sorry for the delay. And thank you very much for the contribution.

@aralroca aralroca self-requested a review November 7, 2021 15:20
@dndhm
Copy link
Contributor Author

dndhm commented Nov 7, 2021

No worries - let me know if you have any feedback I can help out with!

Copy link
Owner

@aralroca aralroca left a comment

Choose a reason for hiding this comment

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

Thank you very much for your contribution! 🤗

Sorry for the delay in reviewing the PR. I see the PR very well 😊

I think it's good that there is a different component just for this and not give a thousand options to the Trans component, so who uses the Trans does not import code and avoid conflicts with i18nKey and text. Therefore, perfect👏

Do you think it would be too much to ask to add in the README information about the TransText component 🤔? If it is too much trouble I can merge the PR and we will do it.

@dndhm
Copy link
Contributor Author

dndhm commented Nov 16, 2021

Hi @aralroca - sure, no problem! Thanks for the kind review. Looking forward to getting this merged.

I'll add a README update to this PR in the next day or two.

@aralroca aralroca merged commit 98149f0 into aralroca:canary Nov 17, 2021
@aralroca
Copy link
Owner

@all-contributors please add @dndhm for code, test and documentation

@allcontributors
Copy link
Contributor

@aralroca

I've put up a pull request to add @dndhm! 🎉

@aralroca
Copy link
Owner

Thanks @dndhm !! I prereleased on 1.3.0-canary.3. It will be finally released on the next release 1.3.0

@aralroca aralroca modified the milestones: future, next release Nov 17, 2021
@dndhm dndhm deleted the feature/add-trans-text branch November 17, 2021 19:23
@dndhm
Copy link
Contributor Author

dndhm commented Nov 17, 2021

Great, thanks! Happy to be involved! 🎉

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

3 participants