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

doc: relocate CII badge #13283

Closed
wants to merge 2 commits into from
Closed

doc: relocate CII badge #13283

wants to merge 2 commits into from

Conversation

refack
Copy link
Contributor

@refack refack commented May 29, 2017

Follow up to #13231 (comment)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label May 29, 2017
@refack
Copy link
Contributor Author

refack commented May 29, 2017

@cjihrig
Copy link
Contributor

cjihrig commented May 29, 2017

I don't think you want to move the Node logo to the bottom of the README

@Fishrock123
Copy link
Contributor

Can you leave the logo up top?

@refack
Copy link
Contributor Author

refack commented May 29, 2017

Sorry about the logo, already pushed fix.

@Fishrock123
Copy link
Contributor

Also, isn't this down a bit far? What about just under the first paragraph-ish?

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM, but to be honest, I think it looks a little out of place and might be better off removed.

@refack
Copy link
Contributor Author

refack commented May 29, 2017

Also, isn't this down a bit far? What about just under the first paragraph-ish?

IMHO it works perfectly as a footnote (but I'm admittedly design-blind). Let's see what others think.

@refack refack self-assigned this May 29, 2017
@gibfahn
Copy link
Member

gibfahn commented May 29, 2017

Looks fine there to me. From #6819 it sounds like it was some work to get it, so we might as well keep it.

@lpinca
Copy link
Member

lpinca commented May 29, 2017

We are essentially hiding it which doesn't makes sense imo. I think the scope of a badge is to be visible.

@refack refack force-pushed the relocate-cii branch 2 times, most recently from f28d611 to abdbfff Compare May 29, 2017 22:25
@refack
Copy link
Contributor Author

refack commented May 29, 2017

Also, isn't this down a bit far? What about just under the first paragraph-ish?

We are essentially hiding it which doesn't makes sense imo. I think the scope of a badge is to be visible.

Commit No. 2 puts it in the bottom of the "preamble"

You can compare the two commits
No. 1
No. 2

@lpinca
Copy link
Member

lpinca commented May 30, 2017

It still seems misplaced to me but definitely better than before.

@gibfahn
Copy link
Member

gibfahn commented May 30, 2017

It still seems misplaced to me but definitely better than before.

Where do you think it should be?

@refack
Copy link
Contributor Author

refack commented May 30, 2017

I tried to put it inline in the preamble; something like:


This project follows the best parctises of the CII image


But as you can see, as usual images don't go well inline :(

@lpinca
Copy link
Member

lpinca commented May 30, 2017

@gibfahn I think below the logo, where it is now, is the best place maybe left aligned instead of centered.

@gibfahn
Copy link
Member

gibfahn commented May 30, 2017

@gibfahn I think below the logo, where it is now, is the best place maybe left aligned instead of centered.

Fair enough, maybe we should just leave it where it is then

@refack
Copy link
Contributor Author

refack commented Jul 9, 2017

Consensus seems to be to stay as it

@refack refack closed this Jul 9, 2017
@gibfahn gibfahn mentioned this pull request Feb 21, 2018
3 tasks
@refack refack removed their assignment Oct 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants