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

feat: add prefix suffix config of args #965

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

LittleBadBad
Copy link

  1. add prefix suffix config of args in hardcode
  2. add new react-i18next template
  3. remove the "," in the replacer

@LittleBadBad
Copy link
Author

fixed issues in #964

@LittleBadBad LittleBadBad force-pushed the feat/args-pre-suf branch 3 times, most recently from d0426c6 to f16be95 Compare July 6, 2023 08:19
@LittleBadBad
Copy link
Author

I've already done the e2e test locally, but the test case would cause bunch of test code modifications, so only key modifications were submitted

Copy link
Collaborator

@terales terales left a comment

Choose a reason for hiding this comment

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

@LittleBadBad Thank you for the change!

Can you merge the current main branch, please?
I'm eager to include this in the upcoming release if the comma change is fully backward compatible.

src/frameworks/react-i18next.ts Outdated Show resolved Hide resolved
@@ -1,3 +1,4 @@
import { Config } from '~/core'
Copy link
Collaborator

@terales terales Aug 28, 2023

Choose a reason for hiding this comment

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

Tests are failing with an error:

Error: Cannot find module '~/core'
Require stack:
- /Users/terales/Documents/GitHub/i18n-ally-lokalise/src/extraction/parseHardString.ts

It looks like you can't import it in this way and I'm not sure not how exactly it should be done

Copy link
Author

Choose a reason for hiding this comment

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

image
how about fix in this way, add a getString param to the parseHardString, then find all the usage, and set the getString param to

(args) => `${Config.argsPrefix}${args.length - 1}${Config.argsSuffix}`

Copy link
Collaborator

Choose a reason for hiding this comment

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

Lets try and see how it look,
just call it an argsToStringCallback please

Copy link
Author

Choose a reason for hiding this comment

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

all unit test passed, I'll push this commit

Copy link
Collaborator

@terales terales left a comment

Choose a reason for hiding this comment

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

@LittleBadBad I've tried to extract into {{t('some-key')}} but was not able to, see the video:

2023-09-23_10-34-38.mp4

Am I missing anything?

@LittleBadBad
Copy link
Author

@LittleBadBad I've tried to extract into {{t('some-key')}} but was not able to, see the video:

2023-09-23_10-34-38.mp4
Am I missing anything?

the "args" means the args in the string, like: the ${arg0} in the string, the {arg0} in the string, if the prefix and suffix were set to "{{" and "}}", the i18n value will be: the {{0}} in the string. it can be used when the i18n lib's arg parser detect the arg in the i18n value by some other symbol, like "{{" and "}}", or "${" and "}", this variable can customize the symbol.

@LittleBadBad
Copy link
Author

@LittleBadBad I've tried to extract into {{t('some-key')}} but was not able to, see the video:

2023-09-23_10-34-38.mp4
Am I missing anything?

in your case, you can try put some arg in the string Test string new, like Test string {123} new

@LittleBadBad
Copy link
Author

@LittleBadBad I've tried to extract into {{t('some-key')}} but was not able to, see the video:

2023-09-23_10-34-38.mp4
Am I missing anything?
here is a use case
img_v2_ed902b28-7e10-48c8-8432-4980a63d73eg

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants