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: improve header styling for API docs #8811

Merged
merged 1 commit into from
Oct 21, 2016

Conversation

Fishrock123
Copy link
Contributor

@Fishrock123 Fishrock123 commented Sep 27, 2016

Checklist
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc

Description of change

Improved the header sizing.
Previously I found it consistently hard to find which API I was looking at.

Sizes were mostly decided by using http://type-scale.com/
with the 1.250 "Major Third" scaling.

Live preview at https://fishrock123.github.io/api/

Screenshots available at: https://gist.github.com/Fishrock123/e2a4676535abed7c8ecb72b1ae081f60

@Fishrock123 Fishrock123 added the doc Issues and PRs related to the documentations. label Sep 27, 2016
@lpinca
Copy link
Member

lpinca commented Sep 28, 2016

I actually prefer the old version, but I'm fine with this as well.

@Fishrock123
Copy link
Contributor Author

Note: currently some of the header sizes are smaller than the surrounding text. I could reduce the scaling if necessary but I'm not sure how this could possibly be a step backwards.

@lpinca
Copy link
Member

lpinca commented Sep 28, 2016

currently some of the header sizes are smaller than the surrounding text.

h3 headers are of the same size of the paragraphs which I agree is bad but everything looks so HUGE on the screenshots especially on mobile. Maybe the screenshots are misleading or is just me, don't worry.

@Fishrock123
Copy link
Contributor Author

The mobile screenshots scale up because of how GitHub displays them. My phone is an iPhone 6s with a screen size of something like 4.5 inches.

Your best bet would be to try it locally.

@Fishrock123
Copy link
Contributor Author

(Actually, currently h4 is smaller than h5: https://nodejs.org/dist/latest-v6.x/docs/api/stream.html#stream_class_stream_writable)

@lpinca
Copy link
Member

lpinca commented Sep 28, 2016

Tested on iPhone 5, looks good.

@Fishrock123
Copy link
Contributor Author

maybe @nodejs/website would prefer to review?

@ghost
Copy link

ghost commented Sep 29, 2016

tested on oneplus 3 and mbp retina 2014, looks good

@Fishrock123
Copy link
Contributor Author

@nodejs/collaborators anyone with some CSS knowledge, could you please review?

Copy link
Contributor

@silverwind silverwind left a comment

Choose a reason for hiding this comment

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

Few CSS comments, didn't test the changes yet.

@@ -15,6 +15,31 @@ body {
background: #fff;
}

h1, h2, h3, h4, h5, h6 {
margin: 0.8em 0 0.5em;
Copy link
Contributor

@silverwind silverwind Oct 3, 2016

Choose a reason for hiding this comment

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

Unnecessary leading zeroes. Can you fix them here and everywhere in this file?


h5, h6 {
margin: 1em 0 0.8em;
line-height: 1.2;
Copy link
Contributor

Choose a reason for hiding this comment

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

this rule seems superfluous (see rule above).

@@ -338,7 +338,7 @@ hr {
margin-bottom: 0;
}

p tt, p code, li code {
p tt, p code, li code, h2 code, h3 code, h4 code, h5 code {
Copy link
Contributor

@silverwind silverwind Oct 3, 2016

Choose a reason for hiding this comment

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

Care to investigate if we can reduce this selector to tt, code? It's a bit hackish having to specify all the different ancestors.

@Fishrock123
Copy link
Contributor Author

Updated, ptal @silverwind

The one ruleset is now for all tt, code { -- didn't seem to make an difference but perhaps there's some obscure spot where it makes a difference.

@Fishrock123
Copy link
Contributor Author

ping again @silverwind

@silverwind
Copy link
Contributor

silverwind commented Oct 6, 2016

LGTM, but one more suggestion: Can you increase the font size of the TOC by a notch? The difference between TOC entries and body/headings seems a bit too much. Or reduce the sizes of those slightly. Your call.

@Fishrock123
Copy link
Contributor Author

Fishrock123 commented Oct 6, 2016

@silverwind which TOC do you mean? The sidebar? The "index" page, or what comes at the top of every page?

If you mean the one on every page, did you just mean the "table of Contents" Header, or the actual entries?

@silverwind
Copy link
Contributor

I mean the links on top here:

Fishrock123 added a commit to Fishrock123/fishrock123.github.io that referenced this pull request Oct 14, 2016
@Fishrock123
Copy link
Contributor Author

Fishrock123 commented Oct 14, 2016

I've put this up at https://fishrock123.github.io/api/ so others can view what it's like to scroll etc with it

@rvagg
Copy link
Member

rvagg commented Oct 17, 2016

Now I can cruise through examples I like the increased font sizes and the clarity of the difference between header sizes makes a lot of difference too.

lgtm

@nodejs/website last chance to pop in and give an opinion here

Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

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

LGTM

@phillipj
Copy link
Member

LGTM

Copy link
Contributor

@princejwesley princejwesley left a comment

Choose a reason for hiding this comment

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

LGTM

Sizes were mostly decided by using http://type-scale.com/
with the 1.250 "Major Third" scaling.

PR-URL: nodejs#8811
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Prince John Wesley <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Phillip Johnsen <[email protected]>
@Fishrock123
Copy link
Contributor Author

Thanks, landed in f1a3755

@Fishrock123 Fishrock123 merged commit f1a3755 into nodejs:master Oct 21, 2016
jasnell pushed a commit that referenced this pull request Oct 24, 2016
Sizes were mostly decided by using http://type-scale.com/
with the 1.250 "Major Third" scaling.

PR-URL: #8811
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Prince John Wesley <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Phillip Johnsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 15, 2016
Sizes were mostly decided by using http://type-scale.com/
with the 1.250 "Major Third" scaling.

PR-URL: #8811
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Prince John Wesley <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Phillip Johnsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 15, 2016
Sizes were mostly decided by using http://type-scale.com/
with the 1.250 "Major Third" scaling.

PR-URL: #8811
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Prince John Wesley <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Phillip Johnsen <[email protected]>
This was referenced Nov 22, 2016
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