-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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: make the documentation print-friendly #6748
Conversation
LGTM, but please add |
Sorry, I meant the commit itself, not just the PR. You'll need to do a |
/cc @nodejs/website |
@Qard I did --ammend, and --force push |
overflow-y : auto; | ||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: missing newline at the end of file.
@ChALkeR I updated the commit. |
margin-left: auto; | ||
overflow-y: auto; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New line still missing here.
Please edit the commit message while you are at it: Make -> make. |
|
||
|
||
@media print { | ||
html { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace
html {
with
html {
as I noted above (one space between html
and {
instead of two).
White spaces removed. |
Has something remained? |
@sorcamarian I think this is good to go. Like @lpinca said in his last comment, the commit message subject should be all-lowercase, but things like this can be taken of when landing the commit. If you’re asking why this isn’t merged yet, it’s just that it’s customary to wait a bit (72 hours over weekends is a general rule) before merging so that everyone has a chance to look at it. I’d suggest you additionally use Also: You author name in this commit is given as “Marian”. Is that intended or do you prefer to be listed (changelog, git log, AUTHORS file) with some other name? People typically prefer their full name, but ultimately it’s up to you. |
@addaleax Thanks for the given explanations. I'm fine with the current name("Marian"). |
LGTM... minor nits regarding newlines + commit message can be fixed when landing Would like to hear something from more folks from @nodejs/documentation and @nodejs/website |
All nits seem to be fixed. |
LGTM |
Any news? |
Fixes: #6743 PR-URL: #6748 Reviewed-By: Robert Jefe Lindstaedt <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Myles Borins <[email protected]>
Landed in c161849. Thanks! |
Checklist
Affected core subsystem(s)
Description of change