Skip to content

doc: add logo to README#12148

Closed
silverwind wants to merge 2 commits intonodejs:masterfrom
silverwind:readme-logo
Closed

doc: add logo to README#12148
silverwind wants to merge 2 commits intonodejs:masterfrom
silverwind:readme-logo

Conversation

@silverwind
Copy link
Contributor

@silverwind silverwind commented Mar 31, 2017

Checklist
  • documentation is changed or added
  • commit message follows commit guidelines
Description

Added a centered SVG logo to the README to make it a little more festive. As centering is not possible in pure Markdown, I had to use a bit of HTML.

Ref: #6920

Preview

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Mar 31, 2017
@silverwind silverwind mentioned this pull request Mar 31, 2017
Closed
gibfahn
gibfahn approved these changes Mar 31, 2017
View reviewed changes
Copy link
Member

@gibfahn gibfahn left a comment

Choose a reason for hiding this comment

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

Looks very nice

@addaleax
Copy link
Member

addaleax commented Mar 31, 2017

You made that SVG, right? Have you considered uploading it as a newer/better version of https://en.wikipedia.org/wiki/File:Node.js_logo.svg?

@TimothyGu
Copy link
Member

TimothyGu commented Mar 31, 2017 •
edited
Loading

@addaleax the logos are all in https://github.com/nodejs/nodejs.org/blob/master/static/images/. The Illustrator files from which SVGs can be generated, are from Amanda Cohen by way of @gtewallace (ref nodejs/evangelism#179).

TimothyGu
TimothyGu reviewed Mar 31, 2017
View reviewed changes
README.md Outdated
[![Gitter](https://badges.gitter.im/Join%20Chat.svg)](https://gitter.im/nodejs/node?utm_source=badge&utm_medium=badge&utm_campaign=pr-badge&utm_content=badge) [![CII Best Practices](https://bestpractices.coreinfrastructure.org/projects/29/badge)](https://bestpractices.coreinfrastructure.org/projects/29)
<p align="center">
<a href="https://nodejs.org/">
<img src="https://nodejs.org/static/images/logo-light.svg" width="400" />
Copy link
Member

@TimothyGu TimothyGu Mar 31, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

An alt attribute seems appropriate here for screen readers (ditto below).

Copy link
Member

@Trott Trott Apr 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, should have an alt attribute and it should probably be empty: alt=""

Copy link
Contributor Author

@silverwind silverwind Apr 1, 2017

Choose a reason for hiding this comment

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

Why not alt="Node.js logo"?

Copy link
Member

@Trott Trott Apr 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

We should follow guidance of things like http://webaim.org/techniques/alttext/#logos. "Node.js home page" is probably what we want per that doc. Specifically, the alt text should likely not contain the word "logo".

Copy link
Contributor Author

@silverwind silverwind Apr 1, 2017

Choose a reason for hiding this comment

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

I think I'll just go with alt="Node.js" and remove the link on the logo as there are already multiple links in the document that link to the homepage.

@ghost
Copy link

ghost commented Apr 1, 2017

Good idea

@silverwind
Copy link
Contributor Author

silverwind commented Apr 1, 2017

@addaleax updated the image on Wikimedia.

@silverwind
Copy link
Contributor Author

silverwind commented Apr 2, 2017

Landed in 2d039ff 🎉

@silverwind silverwind closed this Apr 2, 2017
silverwind added a commit that referenced this pull request Apr 2, 2017
@silverwind
doc: add logo to README
2d039ff 
Adds a centered logo to the README to make it a little more festive. As
centering is not possible in pure Markdown, a bit of HTML is used.

PR-URL: #12148
Ref: #6920
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@silverwind
Copy link
Contributor Author

silverwind commented Apr 3, 2017

I noticed that GitHub's Markdown renderer did wrap the logo in a <a> tag linking to the image source. Probably better to add our own <a> to override that.

@jasnell jasnell mentioned this pull request Apr 4, 2017
Closed
italoacasas pushed a commit to italoacasas/node that referenced this pull request Apr 10, 2017
@silverwind
doc: add logo to README
b5283f9 
Adds a centered logo to the README to make it a little more festive. As
centering is not possible in pure Markdown, a bit of HTML is used.

PR-URL: nodejs#12148
Ref: nodejs#6920
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@italoacasas italoacasas mentioned this pull request Apr 10, 2017
Merged
2 tasks
MylesBorins pushed a commit that referenced this pull request Apr 18, 2017
@silverwind @MylesBorins
doc: add logo to README
b5c9c12 
Adds a centered logo to the README to make it a little more festive. As
centering is not possible in pure Markdown, a bit of HTML is used.

PR-URL: #12148
Ref: #6920
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
@silverwind @MylesBorins
doc: add logo to README
d007427 
Adds a centered logo to the README to make it a little more festive. As
centering is not possible in pure Markdown, a bit of HTML is used.

PR-URL: #12148
Ref: #6920
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Apr 19, 2017
Merged
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
@silverwind @andrew749
doc: add logo to README
74a990f 
Adds a centered logo to the README to make it a little more festive. As
centering is not possible in pure Markdown, a bit of HTML is used.

PR-URL: nodejs/node#12148
Ref: nodejs/node#6920
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Trott Trott Trott left review comments

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

@richardlau richardlau richardlau approved these changes

+4 more reviewers

@TimothyGu TimothyGu TimothyGu left review comments

@hiroppy hiroppy hiroppy approved these changes

@italoacasas italoacasas italoacasas approved these changes

@gibfahn gibfahn gibfahn approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

doc Issues and PRs related to the documentations.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.