-
-
Notifications
You must be signed in to change notification settings - Fork 77
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
Update title/description of env vars in MSI installer #945
Conversation
@jmjaffe37 FYI |
<String Id="FeatureJavaHomeTitle" Value="Set JAVA_HOME variable" /> | ||
<String Id="FeatureJavaHomeDescription" Value="Set JAVA_HOME environment variable." /> | ||
<String Id="FeatureEnvironmentTitle" Value="Modify PATH variable" /> | ||
<String Id="FeatureEnvironmentDescription" Value="Modify PATH environment variable by prepending the JDK installation directory to the beginning of PATH." /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels pretty long? Have you built the MSI locally or downloaded one built in the GitHub workflow to see if this actually fits in the box?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should fit based on a visual assessment :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gdams, @brunoborges, here are some screenshots (ignore the logo, I made these locally just to test the visuals of the new wording):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on previous test, it must fit:
#732 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@douph1 are you able to provide the french text updates for this PR?
Also ping @hendrikebbers @CarmenDelgadoEclipse and @zdtsw who may be able to help with the other locales
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is that to have a PR for https://github.com/brunoborges/installer/blame/patch-1/wix/Lang/OpenJDK.Base.zh-cn.wxl.template on line 10-13?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zdtsw I believe it is easier by looking at the diff of this PR. There are now a total of 5 lines being modified here. They are related to the following Id
items:
- FeatureEnvironmentTitle
- FeatureEnvironmentDescription
- FeatureJavaHomeTitle
- FeatureJavaHomeDescription
- BrowseDlg_Title
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ref: #951
Would be good to land this regardless of the translations (which can follow separately I think), especially because of the extra a11y fix. |
This PR only covers English.
I'd appreciate others' contributions for other languages.