-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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
build: update Windows icon to Feb 2016 node rebrand #28524
Conversation
Hi @nodejs-github-bot , this isn't C++. I suspect you're not very clever though and won't understand this message. |
@mikemaccana can you update the commit message to adhere to our commit guidelines? i think
would do the trick |
Sure @devsnek, done. |
Just noticed the icon has some minor imperfections at the top and bottom-right edges of the hexagon which are also present in the source SVG, no idea how I could've missed those. I'll have a look at fixing them. |
@silverwind Do you mean in the source file at https://github.com/nodejs/nodejs.org/blob/master/static/images/logo-hexagon.svg? Yeah I noticed those too. Here's a screenshot for anyone else: They'll affect people using the node logo at high resolution in banners / slideshows at events etc. but theyy don't have much of an effect att 256 x 256 (this PR). Worth making a separate issue for them. |
Yes, the full logo does not show the issue, so it must've been introduced when extracting the hexagon from it. I'll have a PR against the website up soon with a fix, ideally you could then regenerate the .ico from it. |
@silverwind no probs, done. |
Yep, 256 is used on high resolution displays. See the Windows Icon guidelines:
|
Edit: It's mentioned in #27934 (comment) |
I believe there's nothing that our CI could detect here, so I'll mark it as author-ready even without a CI run. |
If someone on Windows could verify that the built node.exe gets the right icon, I think this is good to land. |
@nodejs/platform-windows |
Fixes #27934