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

Added required UIRef for localized strings #8884

Closed
wants to merge 1 commit into from
Closed

Added required UIRef for localized strings #8884

wants to merge 1 commit into from

Conversation

billti
Copy link
Contributor

@billti billti commented Oct 1, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Description of change

Installing the latest builds on Windows, I noticed the installer was display incorrect strings for the status text on the progress page. Digging into this a little I found the root cause.

It appears localization work has started on the installer (even though still only for en-us at the moment). However a UIRef element required for localized errors and progress text is missing, causing the progress status messages (and others) in the installer to display the template string incorrectly.

This fix simply adds the required UIRef as outlined at the end of the doc page at http://wixtoolset.org/documentation/manual/v3/wixui/wixui_localization.html, or in the StackOverflow answer at http://stackoverflow.com/a/5986030/1674945 .

Below shows the progress dialog before and after the change.

Before

nodeinstaller

After

nodeinstallerfixed

@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Oct 1, 2016
@mscdex mscdex added windows Issues and PRs related to the Windows platform. install Issues and PRs related to the installers. labels Oct 1, 2016
@fhinkel
Copy link
Member

fhinkel commented Oct 3, 2016

Thanks! Do you mind changing the commit message?

tools: add required UIRef for localized strings

@fhinkel
Copy link
Member

fhinkel commented Oct 3, 2016

@billti
Copy link
Contributor Author

billti commented Oct 3, 2016

I've updated the commit msg.

Not sure why the CI failed on an ARM build for Linux as this only touches the MSI installer for Windows... seems unrelated.

@billti
Copy link
Contributor Author

billti commented Oct 6, 2016

ping... @fhinkel any thing else you need on this?

@rvagg rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02
@billti
Copy link
Contributor Author

billti commented Dec 14, 2016

This simple fix has been out a couple month... anything else needed here?

@sam-github
Copy link
Contributor

@nodejs/platform-windows

Copy link
Member

@joaocgreis joaocgreis left a comment

Choose a reason for hiding this comment

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

I can't reproduce the original issue as I don't have a non-english OS at hand, but the fix makes sense.

I've tested a MSI build with this on Windows 2008R2 and Windows 10 and did not see any issue.

@joaocgreis
Copy link
Member

Landed in 7a0fe9f

@billti Thanks!

@joaocgreis joaocgreis closed this Dec 20, 2016
joaocgreis pushed a commit that referenced this pull request Dec 20, 2016
Reviewed-By: João Reis <reis@janeasystems.com>
PR-URL: #8884
cjihrig pushed a commit to cjihrig/node that referenced this pull request Dec 20, 2016
Reviewed-By: João Reis <reis@janeasystems.com>
PR-URL: nodejs#8884
@italoacasas italoacasas mentioned this pull request Dec 20, 2016
cjihrig pushed a commit that referenced this pull request Dec 20, 2016
Reviewed-By: João Reis <reis@janeasystems.com>
PR-URL: #8884
MylesBorins pushed a commit that referenced this pull request Jan 22, 2017
Reviewed-By: João Reis <reis@janeasystems.com>
PR-URL: #8884
@MylesBorins
Copy link
Contributor

@joaocgreis I've backported to v4 and v6, let me know if we should not land it.

MylesBorins pushed a commit that referenced this pull request Jan 23, 2017
Reviewed-By: João Reis <reis@janeasystems.com>
PR-URL: #8884
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
Reviewed-By: João Reis <reis@janeasystems.com>
PR-URL: #8884
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
Reviewed-By: João Reis <reis@janeasystems.com>
PR-URL: #8884
This was referenced Jan 24, 2017
MylesBorins pushed a commit that referenced this pull request Jan 31, 2017
Reviewed-By: João Reis <reis@janeasystems.com>
PR-URL: #8884
MylesBorins pushed a commit that referenced this pull request Feb 1, 2017
Reviewed-By: João Reis <reis@janeasystems.com>
PR-URL: #8884
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
install Issues and PRs related to the installers. tools Issues and PRs related to the tools directory. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants