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: fix minor style issue in code examples #9482

Closed

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Nov 5, 2016

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

doc

Description of change

I've noticed that a few of the code examples have an minor indentation
issue with the first line, for example:
https://nodejs.org/api/child_process.html#child_process_child_process

This commit attempt to fix this issue by using the suggestion provided
by Brain White which is to add a separate style for code with a
padding-right and padding-left of .0em.

Fixes: #9381

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Nov 5, 2016
@mscdex
Copy link
Contributor

mscdex commented Nov 5, 2016

The commit message should include: Fixes: https://github.com/nodejs/node/issues/9381

@mscdex
Copy link
Contributor

mscdex commented Nov 5, 2016

Also, I think it might be more involved than just changing the one padding value. Changing the value also affects the tt tag formatting and IMHO those tags look better with the current padding.

Perhaps we could just keep the existing tt, code block the same, but add a block afterwards like:

code {
  padding: .1em .0em !important;
}

or strip the padding from the tt, code block and just add a new, separate block for each tag, with the correct padding in each.

@danbev
Copy link
Contributor Author

danbev commented Nov 6, 2016

The commit message should include: Fixes: #9381

I'll make sure I take a look at any existing issue next time. I'll add this. Thanks

@danbev danbev force-pushed the fix-style-issue-in-code-examples branch from 4d46d53 to 63c3f8a Compare November 6, 2016 19:18
@danbev
Copy link
Contributor Author

danbev commented Nov 6, 2016

Perhaps we could just keep the existing tt, code block the same, but add a block afterwards like

Thanks, I've updated the PR with your solution. Let me know if there is anything I've missed.

@Fishrock123
Copy link
Contributor

Erm, is there no way to do this without !important? That smells of other structure problems in the CSS file.

@danbev
Copy link
Contributor Author

danbev commented Nov 8, 2016

Erm, is there no way to do this without !important? That smells of other structure problems in the CSS file.

My CSS skills are definitely questionable so please let me know if you have something different in mind. I've updated the PR with the other suggestion provided by @mscdex. Having a separate tt and code selector with the padding for each and no !important.

@silverwind
Copy link
Contributor

silverwind commented Nov 9, 2016

I think the preferable solution would be to eliminate the code nested in pre completely, but I'd accept something like this (with the comment) as a temporary solution:

/* TODO: eliminate the `pre > code` nesting in favor of `pre` */
pre > code {
  padding: 0;
}

But please make sure it doesn't affect anything else first :)

@danbev
Copy link
Contributor Author

danbev commented Nov 10, 2016

@silverwind Thanks for your guidance. I've updated the PR and please let me know what you think.

@danbev danbev mentioned this pull request Nov 10, 2016
2 tasks
@@ -358,7 +360,6 @@ tt, code {
color: #040404;
background-color: #f2f2f2;
border-radius: 2px;
padding: .1em .3em;
Copy link
Contributor

@silverwind silverwind Nov 10, 2016

Choose a reason for hiding this comment

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

Please don't remove this line, pre > code wins over it in terms of rule specificity and this rule still necessary to add padding to inline <code> blocks.

@silverwind
Copy link
Contributor

You can also drop the comment. See #9535 (comment) for my reasoning.

I've noticed that a few of the code examples have an minor
indentation issue with the first line, for example:
https://nodejs.org/api/child_process.html#child_process_child_process

This commit attempt to fix this issue by using the solution provided
provided by silverwind and hiendv.

Fixes: nodejs#9381
@danbev danbev force-pushed the fix-style-issue-in-code-examples branch from 73fda06 to bb57a57 Compare November 10, 2016 19:23
@silverwind
Copy link
Contributor

Thanks! Landed in bd83422.

@silverwind silverwind closed this Nov 11, 2016
silverwind pushed a commit that referenced this pull request Nov 11, 2016
I've noticed that a few of the code examples have an minor
indentation issue with the first line, for example:

https://nodejs.org/api/child_process.html#child_process_child_process
This commit attempt to fix this issue by using the solution provided
provided by silverwind and hiendv.

Fixes: #9381
PR-URL: #9482
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
addaleax pushed a commit that referenced this pull request Nov 22, 2016
I've noticed that a few of the code examples have an minor
indentation issue with the first line, for example:

https://nodejs.org/api/child_process.html#child_process_child_process
This commit attempt to fix this issue by using the solution provided
provided by silverwind and hiendv.

Fixes: #9381
PR-URL: #9482
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
MylesBorins pushed a commit that referenced this pull request Dec 13, 2016
I've noticed that a few of the code examples have an minor
indentation issue with the first line, for example:

https://nodejs.org/api/child_process.html#child_process_child_process
This commit attempt to fix this issue by using the solution provided
provided by silverwind and hiendv.

Fixes: #9381
PR-URL: #9482
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
MylesBorins pushed a commit that referenced this pull request Dec 13, 2016
I've noticed that a few of the code examples have an minor
indentation issue with the first line, for example:

https://nodejs.org/api/child_process.html#child_process_child_process
This commit attempt to fix this issue by using the solution provided
provided by silverwind and hiendv.

Fixes: #9381
PR-URL: #9482
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
I've noticed that a few of the code examples have an minor
indentation issue with the first line, for example:

https://nodejs.org/api/child_process.html#child_process_child_process
This commit attempt to fix this issue by using the solution provided
provided by silverwind and hiendv.

Fixes: #9381
PR-URL: #9482
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
I've noticed that a few of the code examples have an minor
indentation issue with the first line, for example:

https://nodejs.org/api/child_process.html#child_process_child_process
This commit attempt to fix this issue by using the solution provided
provided by silverwind and hiendv.

Fixes: #9381
PR-URL: #9482
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
This was referenced Dec 21, 2016
@danbev danbev deleted the fix-style-issue-in-code-examples branch January 17, 2017 07:23
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.

code highlight slightly misaligned
7 participants