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 code display in header glitch #31460

Closed
wants to merge 1 commit into from
Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented Jan 22, 2020

Fixes: #31451

@GeoffreyBooth @nodejs/website

Before:

before

After:

after

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Jan 22, 2020
@himself65
Copy link
Member

I think the background-color be dark gray is better

image

@XhmikosR
Copy link
Contributor

Not sure if this really fixes the linked issue since AFAICT it's about rendering code blocks differently.

That being said, not only padding but like most of the other properties could/should be removed. I don't have the time to dig into it more, but you should try:

style.css
h1 code, h2 code, h3 code, h4 code, h5 code, h6 code {
- font-weight: 700;
- line-height: inherit;
- position: relative;
- margin: 1.5rem 0 1rem;
- padding: inherit;
- text-rendering: optimizeLegibility;
}

which means separate the selectors like it was before your other patch.

@Trott
Copy link
Member Author

Trott commented Jan 22, 2020

I think the background-color be dark gray is better

That was intentionally omitted in headers, but now I can't remember why. I have a note-to-self to go back and see about making code blocks use a monospace font in headers, though.

@Trott
Copy link
Member Author

Trott commented Jan 22, 2020

Not sure if this really fixes the linked issue since AFAICT it's about rendering code blocks differently.

The main complaint there, I think, is the big jarring spacing. So it fixes that. Minimal incremental change.

@Trott
Copy link
Member Author

Trott commented Jan 22, 2020

I think the background-color be dark gray is better

That was intentionally omitted in headers, but now I can't remember why. I have a note-to-self to go back and see about making code blocks use a monospace font in headers, though.

I think it was omitted in headers because then every single method and property would have the background and it was kind of jarring in headers. There might have also been an accessibility/contrast issue, but I'm not sure. We can try it, but I'd really greatly prefer to have that be separate from this spacing issue. The spacing issue should definitely be fixed immediately. The background color is a different thing and we can iterate that over time.

@Trott
Copy link
Member Author

Trott commented Jan 22, 2020

That being said, not only padding but like most of the other properties could/should be removed. I don't have the time to dig into it more, but you should try:

style.css
h1 code, h2 code, h3 code, h4 code, h5 code, h6 code {
- font-weight: 700;
- line-height: inherit;
- position: relative;
- margin: 1.5rem 0 1rem;
- padding: inherit;
- text-rendering: optimizeLegibility;
}

which means separate the selectors like it was before your other patch.

Happy to try that (and happier to have someone who knows what they're doing try that) but I'd really like to just get the spacing fixed with as few unanticipated side effects as possible, so I'd prefer this go in first and then someone (possibly me) can experiment with further revisions.

@Trott
Copy link
Member Author

Trott commented Jan 22, 2020

I think the background-color be dark gray is better

That was intentionally omitted in headers, but now I can't remember why. I have a note-to-self to go back and see about making code blocks use a monospace font in headers, though.

I think it was omitted in headers because then every single method and property would have the background and it was kind of jarring in headers. There might have also been an accessibility/contrast issue, but I'm not sure. We can try it, but I'd really greatly prefer to have that be separate from this spacing issue. The spacing issue should definitely be fixed immediately. The background color is a different thing and we can iterate that over time.

Oh, you know what? It might have been the padding had a grey background too so all the entries had this weird trail of grey background. So yeah, after this change, the background color change may work just fine.....

@XhmikosR
Copy link
Contributor

The main complaint there, I think, is the big jarring spacing. So it fixes that. Minimal incremental change.

Either way, one should look into splitting the selectors and removing redundant properties at some point. Not super-important but it should be done eventually.

Just my 2 cents.

Happy to try that (and happier to have someone who knows what they're doing try that) but I'd really like to just get the spacing fixed with as few unanticipated side effects as possible, so I'd prefer this go in first and then someone (possibly me) can experiment with further revisions.

I can try to make a patch, I just don't have the time to build the docs so I can only quickly verify the changes in my browser.

@Trott
Copy link
Member Author

Trott commented Jan 22, 2020

I just don't have the time to build the docs so I can only quickly verify the changes in my browser.

FWIW, make doc-only should run reasonably quickly. Takes about 40 seconds on my 7-year-old laptop. (make doc on the other hand...that can take some time....)

@XhmikosR
Copy link
Contributor

Yeah, my main dev VM is Windows. I know how to make it work, but I'd rather if things were truly cross-platform.

Regardless, this PR does fix the weird space issue which was caused by inheriting the padding, so LGTM.

An untested cleanup patch is here: https://github.com/XhmikosR/node/compare/master...XhmikosR-patch-1

@GeoffreyBooth
Copy link
Member

Personally I expect a code block in a heading to look similarly to how GitHub renders them:

Heading with code

As in, code blocks in headings shouldn't be different from code blocks in body text, other than the font size.

@Trott
Copy link
Member Author

Trott commented Jan 22, 2020

Personally I expect a code block in a heading to look similarly to how GitHub renders them:

Heading with code

As in, code blocks in headings shouldn't be different from code blocks in body text, other than the font size.

Agreed. I'd prefer that be a subsequent enhancement.

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 23, 2020
Trott added a commit to Trott/io.js that referenced this pull request Jan 24, 2020
Fixes: nodejs#31451

PR-URL: nodejs#31460
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
Copy link
Member Author

Trott commented Jan 24, 2020

Landed in d32a715

@Trott Trott closed this Jan 24, 2020
Trott added a commit to Trott/io.js that referenced this pull request Jan 26, 2020
This enables the grey background for inline code in headers.

Refs: nodejs#31460 (comment)

PR-URL: nodejs#31493
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
codebytere pushed a commit that referenced this pull request Feb 17, 2020
Fixes: #31451

PR-URL: #31460
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
codebytere pushed a commit that referenced this pull request Feb 17, 2020
This enables the grey background for inline code in headers.

Refs: #31460 (comment)

PR-URL: #31493
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@codebytere codebytere mentioned this pull request Feb 17, 2020
codebytere pushed a commit that referenced this pull request Mar 15, 2020
Fixes: #31451

PR-URL: #31460
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
codebytere pushed a commit that referenced this pull request Mar 15, 2020
This enables the grey background for inline code in headers.

Refs: #31460 (comment)

PR-URL: #31493
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
codebytere pushed a commit that referenced this pull request Mar 17, 2020
Fixes: #31451

PR-URL: #31460
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
codebytere pushed a commit that referenced this pull request Mar 17, 2020
This enables the grey background for inline code in headers.

Refs: #31460 (comment)

PR-URL: #31493
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@codebytere codebytere mentioned this pull request Mar 17, 2020
@Trott Trott deleted the padding branch April 14, 2022 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docs: <code> blocks in headings not rendered correctly
10 participants