Skip to content

build: update Windows icon to Feb 2016 node rebrand - #28524

Closed
mikemaccana wants to merge 1 commit into
nodejs:masterfrom
mikemaccana:master
Closed

build: update Windows icon to Feb 2016 node rebrand#28524
mikemaccana wants to merge 1 commit into
nodejs:masterfrom
mikemaccana:master

Conversation

@mikemaccana

@mikemaccanamikemaccana commented Jul 3, 2019•
edited by devsnek
Loading

Copy link
Copy Markdown
Contributor

Fixes#27934

@nodejs-github-botnodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jul 3, 2019
@mikemaccana

mikemaccana commented Jul 3, 2019

Copy link
Copy Markdown
ContributorAuthor

Hi @nodejs-github-bot , this isn't C++. I suspect you're not very clever though and won't understand this message.

@devsnekdevsnek added build Issues and PRs related to build files or the CI. windows Issues and PRs related to the Windows platform. and removed c++ Issues and PRs that require attention from people who are familiar with C++. labels Jul 3, 2019
@devsnek

devsnek commented Jul 3, 2019•
edited
Loading

Copy link
Copy Markdown
Member

@mikemaccana can you update the commit message to adhere to our commit guidelines?

i think

build: update Windows icon to Feb 2016 node rebrand
Refs: https://github.com/nodejs/node/issues/27934

would do the trick

@mikemaccana

mikemaccana commented Jul 3, 2019

Copy link
Copy Markdown
ContributorAuthor

Sure @devsnek, done.

@silverwind

silverwind commented Jul 3, 2019

Copy link
Copy Markdown
Contributor

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.

@mikemaccana

mikemaccana commented Jul 3, 2019•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

@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:

image

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.

@silverwind

silverwind commented Jul 3, 2019

Copy link
Copy Markdown
Contributor

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

silverwind commented Jul 3, 2019

Copy link
Copy Markdown
Contributor

nodejs/nodejs.org#2316

@silverwindsilverwind mentioned this pull request Jul 3, 2019
Merged
@trivikrtrivikr changed the title Update Windows icon to Feb 2016 node rebrand (fixes #27934)build: update Windows icon to Feb 2016 node rebrandJul 3, 2019
@mikemaccana

mikemaccana commented Jul 3, 2019

Copy link
Copy Markdown
ContributorAuthor

ideally you could then regenerate the .ico from it.

@silverwind no probs, done.

@trivikr

trivikr commented Jul 3, 2019

Copy link
Copy Markdown
Member

The original icon is 128x128 (link)
Is there a reason to update it to 256x256 resolution? (link)

@mikemaccana

mikemaccana commented Jul 3, 2019

Copy link
Copy Markdown
ContributorAuthor

Yep, 256 is used on high resolution displays.

See the Windows Icon guidelines:
https://docs.microsoft.com/en-us/windows/desktop/uxguide/vis-icons

Application icons and Control Panel items: The full set includes 16x16, 32x32, 48x48, and 256x256 (code scales between 32 and 256). The .ico file format is required. For Classic Mode, the full set is 16x16, 24x24, 32x32, 48x48 and 64x64.

@silverwind

silverwind commented Jul 4, 2019•
edited
Loading

Copy link
Copy Markdown
Contributor

Which sizes are included here, by the way?

Edit: It's mentioned in #27934 (comment)

@BridgeAR

BridgeAR commented Jul 4, 2019

Copy link
Copy Markdown
Member

I believe there's nothing that our CI could detect here, so I'll mark it as author-ready even without a CI run.

@BridgeARBridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 4, 2019
@silverwind

silverwind commented Jul 4, 2019

Copy link
Copy Markdown
Contributor

If someone on Windows could verify that the built node.exe gets the right icon, I think this is good to land.

@Trott

Trott commented Jul 8, 2019

Copy link
Copy Markdown
Member

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

@bzoz

bzoz commented Jul 9, 2019

Copy link
Copy Markdown
Contributor

With this PR it gets this icon:
image

@silverwind

silverwind commented Jul 9, 2019

Copy link
Copy Markdown
Contributor

Thanks @bzoz. Landed in 012ed49.

silverwind pushed a commit that referenced this pull request Jul 9, 2019
@mikemaccana@silverwind
build: update Windows icon to Feb 2016 rebrand
012ed49
PR-URL: #28524Fixes: #27934
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@BethGriggsBethGriggs added the baking-for-lts PRs that need to wait before landing in a LTS release. label Jul 16, 2019
targos pushed a commit that referenced this pull request Jul 20, 2019
@mikemaccana@targos
build: update Windows icon to Feb 2016 rebrand
608d6ed
PR-URL: #28524Fixes: #27934
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
This was referenced Jul 23, 2019
Closed
Merged
@BethGriggsBethGriggs removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Oct 17, 2019
BethGriggs pushed a commit that referenced this pull request Oct 17, 2019
@mikemaccana@BethGriggs
build: update Windows icon to Feb 2016 rebrand
f1a5a36
PR-URL: #28524Fixes: #27934
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@BethGriggsBethGriggs mentioned this pull request Oct 18, 2019
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TrottTrottTrott approved these changes
@lpincalpincalpinca approved these changes
@BridgeARBridgeARBridgeAR approved these changes
@trivikrtrivikrtrivikr approved these changes
+1 more reviewer
@silverwindsilverwindsilverwind approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.buildIssues and PRs related to build files or the CI.windowsIssues and PRs related to the Windows platform.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Cosmetic: Start menu item installed by Node 12 uses old/outdated node icon

10 participants

@mikemaccana@devsnek@silverwind@trivikr@BridgeAR@Trott@bzoz@lpinca@BethGriggs@nodejs-github-bot