Skip to content

patreon link shouldn't open in the main window #1545

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

Merged
merged 1 commit into from
Aug 1, 2019
Merged

patreon link shouldn't open in the main window #1545

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion locales/en/messages.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -592,7 +592,11 @@
"message": "<p><strong>Betaflight</strong> is a flight controller software that is <strong>open source</strong> and is available free of charge <strong>without warranty</strong> to all users.</p><p>If you found the Betaflight or Betaflight configurator useful, please consider <strong>supporting</strong> its development by donating.</p>"
},
"defaultDonateBottom": {
"message": "<p>If you want to contribute financially on an ongoing basis, you should consider becoming a patron for us on <a href='https://www.patreon.com/betaflight'>Patreon</a>.</p>"
"message": "<p>If you want to contribute financially on an ongoing basis, you should consider becoming a patron for us on $t(patreonLink.message).</p>"
},
"patreonLink": {
"message": "<a href=\"https://www.patreon.com/betaflight\" target=\"_blank\">Patreon</a>",
"description": "Patreon is name, and should not require translation"
Copy link
Member

@McGiverGim McGiverGim Jul 29, 2019

Choose a reason for hiding this comment

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

If the Patreon word and URL must not be translated is better to take it out of the translations file. Some user can translate them without seeing the description and we will not be aware of that if we no look specifically at that element.
I think is better to have them in the js file and pass them as parameter to the original phrase.

Copy link
Member

@McGiverGim McGiverGim Jul 29, 2019

Choose a reason for hiding this comment

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

@mikeller what do you think? I have seen that you approved it, so I'm not sure if my suggestion is the right way.

Copy link
Contributor Author

@Docteh Docteh Jul 29, 2019

Choose a reason for hiding this comment

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

My knowledge of the localization stuff is quite limited, I saw that the string was tagged via an html tag, instead of the usual javascript call, and assumed I had to do everything within the json file.

betaflight-configurator/src/tabs/landing.html

Line 32 in 0b035c1

<div i18n="defaultDonateBottom"></div>

Copy link
Member

@McGiverGim McGiverGim Jul 29, 2019

Choose a reason for hiding this comment

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

True, in this case we don't have the possibility to pass parameters 😭. We can:

  • Modify the i18n code to support it
  • Move the translation from html to js
  • Let as is

Copy link
Contributor Author

@Docteh Docteh Jul 29, 2019

Choose a reason for hiding this comment

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

I guess if we could add the resource before the i18n stuff fires off?

betaflight-configurator/src/js/LogoManager.js

Lines 102 to 106 in 0b035c1

// inject logo size variables for dynamic translation strings
i18n.addResources({
logoWidthPx: "" + this.constraints.imageSize.expectedWidth,
logoHeightPx: "" + this.constraints.imageSize.expectedHeight,
});

betaflight-configurator/locales/en/messages.json

Lines 3751 to 3753 in 0b035c1

"osdSetupCustomLogoInfoImageSize": {
"message": "Size must be $t(logoWidthPx)×$t(logoHeightPx) pixels"
},

Copy link
Member

@mikeller mikeller Jul 30, 2019

Choose a reason for hiding this comment

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

I'm not saying it is going to break anything - I am just not sure that this is a good way to solve this by adding complexity.

Copy link
Member

@McGiverGim McGiverGim Jul 31, 2019

Choose a reason for hiding this comment

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

What I understood in your comment, is that splitting the file into two, needs to change the way that we use to include translations/constants in the HTML/JS. That's not true. The i18next localization that we use will search in both files in a transparent manner for the user if configured correctly. So the developer will use the same instructions/code than before, not matter if it is a translation or a constant, so it does not increase the complexity in any way.

The only drawback, is that the developer must be aware that there are two files, and include the messages in the correct one when adding. Nothing more.

Copy link
Member

@mikeller mikeller Jul 31, 2019

Choose a reason for hiding this comment

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

That still adds complexity over what we currently have - my suggestion is to keep non-translated and translated strings in the same file, as we currently do, and use some form of indicator to mark not-to-be-translated strings as such, and make it part of the CI build job (or potentially even add hooks to crowdin) to check that these constraints are not violated.

Copy link
Member

@mikeller mikeller Aug 1, 2019

Choose a reason for hiding this comment

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

Mergeing this, we can still finish the discussion.

Copy link
Member

@McGiverGim McGiverGim Aug 1, 2019

Choose a reason for hiding this comment

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

I have hidden the Patreon link string in Crowdin, but this is not a perfect solution. The users can show the hidden strings (all the languages strings were hidden and we now some users ended translating some of them).

},
"defaultDonate": {
"message": "Donate"
Expand Down