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 word wrapping for api stability boxes #14809

Closed
wants to merge 3 commits into
base: master
from

Conversation

@saadq
Member

saadq commented Aug 13, 2017

Checklist
Affected core subsystem(s)

doc


This is meant to fix the issue that was opened here originally.

Fixes: nodejs/nodejs.org#1337

Before:
screen shot 2017-08-13 at 12 21 52 pm

After:
screen shot 2017-08-13 at 12 22 02 pm

@lpinca

lpinca approved these changes Aug 13, 2017

@silverwind

This comment has been minimized.

Show comment
Hide comment
@silverwind

silverwind Aug 13, 2017

Contributor

Is there a reason why these boxes are <pre>? Doesn't seem semantically correct to me, and the fact that they inherit rules targeting code blocks isn't ideal either.

I'd change them to <div> and add padding: 1em to them.

Contributor

silverwind commented Aug 13, 2017

Is there a reason why these boxes are <pre>? Doesn't seem semantically correct to me, and the fact that they inherit rules targeting code blocks isn't ideal either.

I'd change them to <div> and add padding: 1em to them.

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack Aug 13, 2017

Member

Is there a reason why these boxes are <pre>? Doesn't seem semantically correct to me, and the fact that they inherit rules targeting code blocks isn't ideal either.

I think it's because it's rendered markdown. A simple style change might be to replace the ``` with >


image

or exclude pre[lang=txt] from the css

Member

refack commented Aug 13, 2017

Is there a reason why these boxes are <pre>? Doesn't seem semantically correct to me, and the fact that they inherit rules targeting code blocks isn't ideal either.

I think it's because it's rendered markdown. A simple style change might be to replace the ``` with >


image

or exclude pre[lang=txt] from the css

@refack

refack approved these changes Aug 13, 2017

@silverwind

This comment has been minimized.

Show comment
Hide comment
@silverwind

silverwind Aug 13, 2017

Contributor

@refack nope, they are generated by our own code:

`<pre class="${classNames}"><a href="${docsUrl}">$1 $2</a>$3</pre>`

Contributor

silverwind commented Aug 13, 2017

@refack nope, they are generated by our own code:

`<pre class="${classNames}"><a href="${docsUrl}">$1 $2</a>$3</pre>`

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack Aug 13, 2017

Member

Mostly enjoying the new code line quoting 👐

@refack nope, they are generated by our own code:

AFAICT that's the header for each API endpoint:

const STABILITY_TEXT_REG_EXP = /(.*:)\s*(\d)([\s\S]*)/;

node/tools/doc/html.js

Lines 397 to 406 in 314217f

function parseAPIHeader(text) {
const classNames = 'api_stability api_stability_$2';
const docsUrl = 'documentation.html#documentation_stability_index';
text = text.replace(
STABILITY_TEXT_REG_EXP,
`<pre class="${classNames}"><a href="${docsUrl}">$1 $2</a>$3</pre>`
);
return text;
}

That maps:

node/doc/api/crypto.md

Lines 678 to 684 in 2e7ccc2

### ecdh.setPublicKey(publicKey[, encoding])
<!-- YAML
added: v0.11.14
deprecated: v5.2.0
-->
> Stability: 0 - Deprecated

to:
image


Anyway let's do both: replace the markup and the rendering code

Member

refack commented Aug 13, 2017

Mostly enjoying the new code line quoting 👐

@refack nope, they are generated by our own code:

AFAICT that's the header for each API endpoint:

const STABILITY_TEXT_REG_EXP = /(.*:)\s*(\d)([\s\S]*)/;

node/tools/doc/html.js

Lines 397 to 406 in 314217f

function parseAPIHeader(text) {
const classNames = 'api_stability api_stability_$2';
const docsUrl = 'documentation.html#documentation_stability_index';
text = text.replace(
STABILITY_TEXT_REG_EXP,
`<pre class="${classNames}"><a href="${docsUrl}">$1 $2</a>$3</pre>`
);
return text;
}

That maps:

node/doc/api/crypto.md

Lines 678 to 684 in 2e7ccc2

### ecdh.setPublicKey(publicKey[, encoding])
<!-- YAML
added: v0.11.14
deprecated: v5.2.0
-->
> Stability: 0 - Deprecated

to:
image


Anyway let's do both: replace the markup and the rendering code

@silverwind

This comment has been minimized.

Show comment
Hide comment
@silverwind

silverwind Aug 13, 2017

Contributor

@refack I'm still pretty sure that <pre> comes from there, the changelog (.api_metadata) is generated elsewhere.

Anyways, my suggestion to @saadq would be to replace the <pre> with a <div> in html.js and add padding: 1em to .api_stability in CSS.

Contributor

silverwind commented Aug 13, 2017

@refack I'm still pretty sure that <pre> comes from there, the changelog (.api_metadata) is generated elsewhere.

Anyways, my suggestion to @saadq would be to replace the <pre> with a <div> in html.js and add padding: 1em to .api_stability in CSS.

@saadq

This comment has been minimized.

Show comment
Hide comment
@saadq

saadq Aug 14, 2017

Member

I agree that pre probably wasn't that semantically correct, so I changed it to div as you suggested. However, this cause some stylistic issues:

After changing pre -> div
screen shot 2017-08-13 at 11 32 29 pm

The font-weight became a lot bolder and the line-height became smaller, so I added a couple of styles to make it look like it did when it was still using pre.

After adding/removing some additional styles
screen shot 2017-08-13 at 11 31 02 pm

Does this look alright to everyone?

Member

saadq commented Aug 14, 2017

I agree that pre probably wasn't that semantically correct, so I changed it to div as you suggested. However, this cause some stylistic issues:

After changing pre -> div
screen shot 2017-08-13 at 11 32 29 pm

The font-weight became a lot bolder and the line-height became smaller, so I added a couple of styles to make it look like it did when it was still using pre.

After adding/removing some additional styles
screen shot 2017-08-13 at 11 31 02 pm

Does this look alright to everyone?

@silverwind

Looking fine to me, thanks.

@Trott Trott dismissed their stale review Aug 14, 2017

A bunch of other people are looking at this more closely and with more expertise than I bring to CSS etc. Plus, my review is from a version of this PR that is now several iterations old. Dismissing my review and letting the knowledgable folks handle it instead. Thanks!

Show outdated Hide outdated doc/api_assets/style.css
@tniessen

This comment has been minimized.

Show comment
Hide comment
@tniessen

tniessen Aug 16, 2017

Member

Landed in eab2bea.

Member

tniessen commented Aug 16, 2017

Landed in eab2bea.

@tniessen tniessen closed this Aug 16, 2017

tniessen added a commit that referenced this pull request Aug 16, 2017

doc: fix word wrapping for api stability boxes
PR-URL: #14809
Fixes: nodejs/nodejs.org#1337
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>

@tniessen tniessen referenced this pull request Aug 18, 2017

Closed

doc: do not compact stability text to one line #14929

2 of 2 tasks complete

MSLaguana added a commit to nodejs/node-chakracore that referenced this pull request Aug 21, 2017

doc: fix word wrapping for api stability boxes
PR-URL: nodejs/node#14809
Fixes: nodejs/nodejs.org#1337
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>

MylesBorins added a commit that referenced this pull request Sep 10, 2017

doc: fix word wrapping for api stability boxes
PR-URL: #14809
Fixes: nodejs/nodejs.org#1337
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>

@MylesBorins MylesBorins referenced this pull request Sep 10, 2017

Merged

v8.5.0 proposal #15308

MylesBorins added a commit that referenced this pull request Sep 12, 2017

doc: fix word wrapping for api stability boxes
PR-URL: #14809
Fixes: nodejs/nodejs.org#1337
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>

MylesBorins added a commit that referenced this pull request Sep 20, 2017

doc: fix word wrapping for api stability boxes
PR-URL: #14809
Fixes: nodejs/nodejs.org#1337
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>

MylesBorins added a commit that referenced this pull request Sep 20, 2017

doc: fix word wrapping for api stability boxes
PR-URL: #14809
Fixes: nodejs/nodejs.org#1337
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>

@MylesBorins MylesBorins referenced this pull request Sep 20, 2017

Merged

v6.11.4 proposal #15506

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment