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

[PRI][WIP] update the JSON format for the translation file en.json #32

Merged
merged 3 commits into from
Dec 22, 2013

Conversation

dan753722
Copy link
Contributor

🐨👍

This PR intends to update the JSON format of the en translation file.

TASKS

@svizzari , @maximeprades, and @tmcinerney can you please review this format? I am happy to further refine it if there is any error :)

/cc @zendesk/quokka

@maximeprades
Copy link
Contributor

👍

@@ -1,6 +1,54 @@
{
"app": {
"description": "Play the famous zen tunes in your help desk.",
"name": "Buddha Machine"
"package": "basic_user_sample",
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, from memory we use this for translation keys. This forms part of the prefix in the generated YML

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a validation that ensures people can't use this default value when packaging their app @liulikun?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes so we either

  1. don't need it here (remember our customers will see this when they run zat new)
  2. need to make it vanilla. like app_name

@tmcinerney
Copy link
Contributor

Can you fix the tests please @dan753722? You might need to check the test fixtures to ensure they are correct.

@tmcinerney
Copy link
Contributor

👍 after tests are green

@liulikun
Copy link
Contributor

@maximeprades @tmcinerney I'm not sure this is a good idea to use zendesk translation format as default. It'll confuse the public users.

@dan753722
Copy link
Contributor Author

@tmcinerney CI is green now :)

@tmcinerney
Copy link
Contributor

Closing since we don't want to complicate the process for app developers, in line with @liulikun comment.

@tmcinerney
Copy link
Contributor

So @maximeprades, would you like this guy merged based on the other discussion?

@maximeprades
Copy link
Contributor

Yes sir
Please

On 21 déc. 2013, at 15:39, Travers McInerney notifications@github.com wrote:

So @maximeprades, would you like this guy merged based on the other discussion?


Reply to this email directly or view it on GitHub.

@tmcinerney tmcinerney reopened this Dec 22, 2013
tmcinerney added a commit that referenced this pull request Dec 22, 2013
[PRI][WIP] update the JSON format for the translation file en.json
@tmcinerney tmcinerney merged commit 13e4a74 into master Dec 22, 2013
@tmcinerney tmcinerney deleted the dansun/update_translation_format_en branch December 22, 2013 00:28
shajith pushed a commit that referenced this pull request May 9, 2019
[PRI][WIP] update the JSON format for the translation file en.json
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

4 participants