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

GSOC 2020: [Preferences] Button problem in Preferences #9185 requires translation #9947

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

yzykov
Copy link

@yzykov yzykov commented Mar 28, 2020

[Preferences] Button problem in Preferences #9185 requires translation

This pull request is more descriptive than #9945, but it requires translation in resource files.
Not sure about necessary workflows.
Tested Windows, Linux.

Tool Tip still pops up for different languages, but on English. I can help with Russian/Ukrainian if needed.

@matthijskooijman
Copy link
Collaborator

At first glance, your changes look good to me, though I have not tested them. I have no strong opinion on the UI changes themselves (so I'll leave those to others to review), but I do have some suggestions for your commit messages:

  • Your first commit states what it does, but not why. What changes when you add this property in the form file? Why is it useful or needed?
  • Your second message only states the problem of the original issue, but the convention here is to state the change the commit makes in the first (short) summary line. In the extended description (after an empty line in the commit message), you can use more works to describe the problem that is fixed and how the fix works. It is also good to reference the ticket number, but ideally not in the summary line but at the end of the commit message, e.g. a separate line saying something like "This commit fixes [Preferences] Button problem in Preferences #9185." which also allows github to autoclose the issue when the PR is merged.

As I previously said elsewhere: If you make these changes, please do a force push to this same branch to update this PR, rather than creating a new PR.

@yzykov
Copy link
Author

yzykov commented Mar 28, 2020

Thank you for such descriptive reply.

Another PR is not closed. It is an alternative solution which does not require translation, but is less descriptive. I am using an existing sentence that was already translated. I did this way, so someone can make a decision about how much effort should be done and which change is preferred.

"What changes when you add this property in the form file?"
I doubt it changes anything because the logic was done at the backend (java file). I was following the existing style. All other sentences were duplicated in both files. .form file are XMl files for IDE, by adding a line I am making it to pop up in XML in the form editor. When java logic is hidden.

At first glance, your changes look good to me, though I have not tested them. I have no strong opinion on the UI changes themselves (so I'll leave those to others to review), but I do have some suggestions for your commit messages:

  • Your first commit states what it does, but not why. What changes when you add this property in the form file? Why is it useful or needed?
  • Your second message only states the problem of the original issue, but the convention here is to state the change the commit makes in the first (short) summary line. In the extended description (after an empty line in the commit message), you can use more works to describe the problem that is fixed and how the fix works. It is also good to reference the ticket number, but ideally not in the summary line but at the end of the commit message, e.g. a separate line saying something like "This commit fixes [Preferences] Button problem in Preferences #9185." which also allows github to autoclose the issue when the PR is merged.

As I previously said elsewhere: If you make these changes, please do a force push to this same branch to update this PR, rather than creating a new PR.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

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