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.

Sign up for GitHub

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

Jump to bottom

js intl #1127

Merged
merged 2 commits into from Sep 6, 2021
Merged

js intl #1127

merged 2 commits into from Sep 6, 2021

Conversation

vctt94
Copy link
Member

@vctt94 vctt94 commented Jul 14, 2021

This PR is a work torward the internationalization.

It is a collective work with #1051 and I am focusing on the js internationalization here.

@buck54321 buck54321 added this to the 0.3 milestone Aug 3, 2021
@vctt94 vctt94 force-pushed the js-intl branch 2 times, most recently from c02bd83 to a017e89 Compare August 4, 2021 15:53
@buck54321 buck54321 mentioned this pull request Aug 6, 2021
Merged
@chappjc
Copy link
Member

chappjc commented Aug 24, 2021

PR #1051 is now merged.

@vctt94 vctt94 force-pushed the js-intl branch 4 times, most recently from 58086b6 to fae17a5 Compare August 25, 2021 20:16
@chappjc chappjc linked an issue Aug 26, 2021 that may be closed by this pull request
client internationalization #1022
Closed
chappjc
chappjc requested changes Aug 26, 2021
View reviewed changes
Copy link
Member

@chappjc chappjc left a comment

Choose a reason for hiding this comment

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

Good progress. Just making a comment about the decision to go with a map entry in the locales package.

Also you'll want to check in the changes to localized_html/en-US/bodybuilder.tmpl

client/core/types.go Outdated Show resolved Hide resolved
client/webserver/locales/en-us.go Outdated Show resolved Hide resolved
chappjc
chappjc reviewed Aug 26, 2021
View reviewed changes
client/core/locale_ntfn.go Outdated Show resolved Hide resolved
@buck54321 buck54321 mentioned this pull request Aug 29, 2021
Closed
@buck54321
Copy link
Member

Can we split off the pt-BR work some time before merging?

@chappjc
Copy link
Member

chappjc commented Aug 30, 2021 •
edited

Also pls squash it down (except with the pt-BR translations separate) before hitting ready for review. I've been testing a little here and there and it's mostly working, except the things we've already chatted about in matrix: a number of notifications in core.go still need to go through formatString, and the map in client/core/locale_htfn.go has to be a map[string]*Translation to get the subject translations too. Thanks, @vctt94

@vctt94 vctt94 force-pushed the js-intl branch 4 times, most recently from 8a11eb1 to b8cd1ef Compare August 31, 2021 16:00
@vctt94 vctt94 mentioned this pull request Sep 1, 2021
Merged
@chappjc chappjc added the i18n Internationalization and translation label Sep 2, 2021
@vctt94 vctt94 force-pushed the js-intl branch 4 times, most recently from 4164320 to a2838f9 Compare September 6, 2021 15:25
chappjc
chappjc requested changes Sep 6, 2021
View reviewed changes
client/core/core.go Outdated Show resolved Hide resolved
client/webserver/site/src/js/locales.js Outdated Show resolved Hide resolved
client/webserver/site/src/js/locales.js Outdated Show resolved Hide resolved
client/webserver/site/src/js/locales.js Show resolved Hide resolved
client/webserver/site/src/js/locales.js Outdated Show resolved Hide resolved
client/webserver/site/src/js/forms.js Outdated Show resolved Hide resolved
client/webserver/site/src/js/forms.js Outdated Show resolved Hide resolved
client/webserver/site/src/js/locales.js
Comment on lines +1 to +5
export const ID_NO_PASS_ERROR_MSG = 'ID_NO_PASS_ERROR_MSG'
export const ID_NO_APP_PASS_ERROR_MSG = 'ID_NO_APP_PASS_ERROR_MSG'
export const ID_SET_BUTTON_BUY = 'ID_SET_BUTTON_BUY'
export const ID_SET_BUTTON_SELL = 'ID_SET_BUTTON_SELL'
export const ID_OFF = 'ID_OFF'
Copy link
Member

@chappjc chappjc Sep 6, 2021 •
edited

Choose a reason for hiding this comment

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

This all seems peculiar to me. Seems like these consts should just be an enumeration or something with integers. Is this really what JavaScript forces us to do? The actual strings here are meaningless, just intended to be unique.

Copy link
Member

Choose a reason for hiding this comment

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

Can leave this if it's Decrediton's approach, unless you or someone else has a better suggestion.

client/webserver/site/src/js/markets.js Show resolved Hide resolved
client/webserver/site/src/js/locales.js Outdated Show resolved Hide resolved
chappjc
chappjc reviewed Sep 6, 2021
View reviewed changes
client/core/core.go Outdated Show resolved Hide resolved
@vctt94 vctt94 force-pushed the js-intl branch 3 times, most recently from 17cd000 to 71c9841 Compare September 6, 2021 21:12
@vctt94 vctt94 changed the title [wip] js intl js intl Sep 6, 2021
@vctt94 vctt94 marked this pull request as ready for review September 6, 2021 21:12
@vctt94 vctt94 force-pushed the js-intl branch 2 times, most recently from 71ce804 to 86d8acd Compare September 6, 2021 21:17
chappjc
chappjc reviewed Sep 6, 2021
View reviewed changes
client/webserver/locales/en-us.go
@@ -165,4 +165,6 @@ var EnUS = map[string]string{
"Restoration Seed": "Restoration Seed",
"Restore from seed": "Restore from seed",
"Import Account": "Import Account",
"no_wallet": "no wallet",
"create_a_x_wallet": "Create a {{.Info.Name}} Wallet", // .Info.Name will be an asset symbol like DCR
Copy link
Member

@chappjc chappjc Sep 6, 2021 •
edited

Choose a reason for hiding this comment

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

To document the reasoning for this, in portuguese we use a different sentence structure from english, saying "criar uma carteira decred" like it was: "create a wallet decred". So doing <button data-action="create">[[[Create a]]] {{.Info.Name}} [[[Wallet]]]</button> created a challenge.
My suggestion was simply to use go html template xpressions in the locale strings and let them find their way in to the localized_html files, which they do. e.g. in pt-BR, <button data-action="create">Criar uma carteira {{.Info.Name}}</button>

Copy link
Member

@chappjc chappjc Sep 6, 2021 •
edited

Choose a reason for hiding this comment

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

Also we can now get rid of the "Create a" map entry, correct? EDIT: resolved.

chappjc
chappjc approved these changes Sep 6, 2021
View reviewed changes
Copy link
Member

@chappjc chappjc left a comment

Choose a reason for hiding this comment

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

Working well. We can discuss tweaks to the translation machinery in #1185, which will follow this.

@chappjc chappjc merged commit 6e6cac4 into decred:master Sep 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chappjc chappjc chappjc approved these changes

Assignees
No one assigned
Labels
i18n Internationalization and translation
Projects
None yet
Milestone
0.3
Development

Successfully merging this pull request may close these issues.

client internationalization
3 participants
@vctt94 @chappjc @buck54321