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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change html manual to use relative font sizes #1767

Merged
merged 10 commits into from May 18, 2018

Conversation

Projects
None yet
9 participants
@charlesetc
Copy link
Contributor

charlesetc commented May 7, 2018

This changes to rem instead of px for the units.

The library documentation is set to 1rem because all the code generation is typeset with tables and they overflow the width of the page very easily.

I've set the manual itself to 1.1rem. I think this small change improves readability enough to matter in the long run but is hopefully small enough that it won't look "big" to those who prefer small fonts. If we reach a different consensus, I'm more than happy change it. As with before in #1757, the there is an up-to-date preview available.

馃巿

@gasche

This comment has been minimized.

Copy link
Member

gasche commented May 7, 2018

(cc @dbuenzli, @dra27)

@charlesetc: Thanks! Could you add tihs GPR number to your previous Changes entry?

git grep font-size from the manual repository returns 6 occurrences, one of which already uses relative sizes. You are moving 3 of them to relative sizes, but what about the remaining two?

.info { margin-left : 3em; margin-right : 3em } .code { color : #465F91 ; } h1 { font-size : 24pt ; text-align: center; }

h2, h3, h4, h5, h6, div.h7, div.h8, div.h9 {
  font-size: 22pt;
  ...
}

I think that it would be nice to also have the header font sizes be relative.

@dbuenzli

This comment has been minimized.

Copy link
Contributor

dbuenzli commented May 7, 2018

I've set the manual itself to 1.1rem. I think this small change improves readability enough to matter in the long run

That's still too big for me (and for a default browser an odd corresponding px size of 17.6px). Really, just use 1rem or at most 1.0625rem (17px) and decrease a bit the page measure. I don't know where the idea came from that bigger font sizes improved readability that's not the way to judge a design's readability which is the balance of many other factors.

I would also avoid changing the font sizes between the API reference and the manual. Be consistent.

@pmetzger

This comment has been minimized.

Copy link
Member

pmetzger commented May 7, 2018

That's still too big for me

And I would prefer it even bigger, with a size like the old 19px selection. :)

@charlesetc A conversion table between px and rem is here: http://www.standardista.com/px-to-rem-conversion-if-root-font-size-is-16px/

I've suggested this elsewhere, but I think we'll need to have several styles available to make everyone happy. Some projects (like Rust) provide a way to do this, we can steal from them. By default, I think we should use a quite large font, but there is no reason not to allow people to use others if they prefer.

I would also avoid changing the font sizes between the API reference and the manual.

I agree that ultimately they should be completely consistent, but right now until we fix some of the ocamldoc generated HTML it will be hard to do. Small steps.

@dbuenzli

This comment has been minimized.

Copy link
Contributor

dbuenzli commented May 7, 2018

but right now until we fix some of the ocamldoc generated HTML it will be hard to do. Small steps.

This is OT but since you have been claiming this for quite some time now I would just like to mention that I and many others have been using an alternate stylesheet (notably visible here) which is perfectly fine using the current ocamldoc generated HTML (it used to require a lot of workaround but thanks to @Octachron's work on the markup most of the quirks are nowadays quite contained).

@pmetzger

This comment has been minimized.

Copy link
Member

pmetzger commented May 7, 2018

This is OT but since you have been claiming this for quite some time now I would just like to mention that I and many others have been using an alternate stylesheet (notably visible here)

So I worked on this for a while and wasn't able to style the type tables so that they looked reasonable without changing around the HTML a bit. I'll look at your work and see if you came up with things I didn't.

However, you needn't wait for me, you can just submit a pull request or suggest specific changes to improve that stuff 鈥 I have no doubt others are more skilled at CSS than me (I'm a rank amateur.)

@pmetzger

This comment has been minimized.

Copy link
Member

pmetzger commented May 7, 2018

Just for reference, @dbuenzli, the main problem I was having (and the main problem I gather @charlesetc is having) is with tables where the comments end up crushing the types on the left, and with font and size selections that won't stick in those tables without kludges like !inherit that cause unintended side effects because the scope of the inherited value ends up impacting unintended parts of the page.

If you have ways to fix this, please please please do help. (We're all in this together.)

@pmetzger

This comment has been minimized.

Copy link
Member

pmetzger commented May 7, 2018

BTW, is it possible that the problems I'm having are because I'm unwittingly using an older ocamldoc on the latest version of the documentation? As has been mentioned, the build process is a bit complicated.

@charlesetc

This comment has been minimized.

Copy link
Contributor Author

charlesetc commented May 7, 2018

So I've changed the text size to 1rem everywhere. I'm also making both the manual and the libref's body smaller and changed it so that libref's body's width works on mobile better and that the type signatures wrap now. (Before long function types would flow out of the page)

items

@dbuenzli

This comment has been minimized.

Copy link
Contributor

dbuenzli commented May 7, 2018

So I've changed the text size to 1rem everywhere.

This looks good to me but it seems that as a result the type in the code fences became to small. I think you should simply remove (or adjust) the font-size: 0.85em in style for pre.

(For example on this page)

@Drup

This comment has been minimized.

Copy link
Contributor

Drup commented May 8, 2018

As someone with terrible eye-sight, I can attest that the last version is well readable (given my font size of 17px).

@gasche

This comment has been minimized.

Copy link
Member

gasche commented May 8, 2018

@charlesetc I noticed that you had updated the remaining non-relative sizes in the css files. Thanks!

Besides making all sizes relatives, two changes are included in this PR:

  • whitespace: pre-wrap on code blocks, which allows line wrapping in code examples (see the example picture in #1767 (comment))
  • a change to the max-width value of the body; I suppose that the goal is to have the max width smaller than the typical mobile phone screen resolution? (I don't really understand why max-width is used for that, I would have thought that CSS allows smaller screens to use a smaller width.)

Finally, you say that you now use 1rem everywhere, but the <pre> style in the manual still uses 0.91rem -- which is larger than the previous 0.85em value.

Are we done? Does anything remain to be discussed before merging?

I grepped more and noticed that ocamldoc still uses non-relative size in its default styles (see git grep font-size at the root of the OCaml repo, using 20pt for title sizes. Given that you use 1.75rem to replace the 22pt non-h1 titles in the manual style, I suppose that 1.6rem would be a reasonable replacement for all the 20pt values. On the other hand, this can also be done in a different PR, possibly with a refresh of the ocamldoc-generated stylesheet (cc @Octachron, @dbuenzli) if warranted.

@charlesetc

This comment has been minimized.

Copy link
Contributor Author

charlesetc commented May 8, 2018

So max-width only affects large screen sizes - on a mobile screen the width never gets there. I made max width smaller to match the decrease in font size. I think @dbuenzli suggested this too with "decrease a bit the page measure." As for the 0.91rem for pre's 鈥 monospaced fonts tend to take up more space than proportional fonts so keeping the font size a little smaller makes everything look even. Happy make it closer to 1 if people think that looks better, I did some experimenting and thought 0.91 looked the best. If this looks good to people we can merge; I'm also happy to add rem values to ocamldoc if we want to do it now. What do people think?

@pmetzger

This comment has been minimized.

Copy link
Member

pmetzger commented May 8, 2018

As for the 0.91rem for pre's 鈥 monospaced fonts tend to take up more space than proportional fonts so keeping the font size a little smaller makes everything look even.

Yes. One reason I keep suggesting that we should be specifying a specific monospace font is that often it is difficult to adjust monospace fonts to match the actual size of the surrounding font without such small adjustments. That said, this will differ from screen to screen because we are not specifying the face but rather allowing the browser to pick one.

Remember that point size is not an exact thing. Two typefaces at the same point size may not be compatible when juxtaposed. Therefore, knowing the exact typeface in use allows one to make sure that the two faces are used in a compatible size.

@charlesetc

This comment has been minimized.

Copy link
Contributor Author

charlesetc commented May 8, 2018

Yeah I completely agree that would make things better. I'm a bit worried that having a bunch of custom fonts will decrease the performance of an otherwise lightweight page which is why I haven't added them in this PR. I think it might take some discussion

width: 85\%;
margin: auto;
background: \#f7f7f7;
margin-top: 80px;
font-size: 19px
font-size: 1rem;
}

This comment has been minimized.

@pmetzger

pmetzger May 8, 2018

Member

18px would be 1.125rem
19px would be 1.1875rem

I'd suggest going for 1.125rem unless this is a problem. Now that we're using relative sizes, people can adjust their browsers to suit.

This comment has been minimized.

@charlesetc

charlesetc May 8, 2018

Author Contributor

@dbuenzli, @pmetzger Seems everyone has strong convictions about this. I'm tempted to revert this piece to keep the original manual's formatting until we can come to consensus. Let's remember that we're talking about a couple pixels and we're all on the same side :) A noble debate nonetheless!

This comment has been minimized.

@dbuenzli

dbuenzli May 8, 2018

Contributor

Oh my I thought that discussion was over. Again, respect the user's defaults.

This comment has been minimized.

@pmetzger

pmetzger May 8, 2018

Member

Hey, my suggestion was smaller than the original 19px! That's a compromise.

If you don't want to do anything, though, leave it 1rem explicitly and we'll reconsider later.

@dbuenzli

This comment has been minimized.

Copy link
Contributor

dbuenzli commented May 8, 2018

Personally I have nothing against merging, thanks for baring with my nits.

However a final one just in case:

Happy make it closer to 1 if people think that looks better, I did some experimenting and thought 0.91 looked the best. If this looks good to people we can merge

Earlier I said remove the font-size in pre because currently you use two sizes for monospace (One in code fences (0.91rem) and another one for monospace in inline text, which seems to be 1rem). I think it would be better to be consistent and use a single size for both cases. On my machine the size in inline text blends better with the serif than the 0.91rem you have chosen (if that's really 1rem, I didn't check, it could even be adjusted a bit larger).

But again I won't fight for this and this can be merged.

@charlesetc

This comment has been minimized.

Copy link
Contributor Author

charlesetc commented May 8, 2018

One in code fences (0.91rem) and another one for monospace in inline text, which seems to be 1rem

I know 馃槥 Problem is the manual doesn't use <code>, it uses spans with an auto generated class so it's hard to change the inline monospace text safely. I'd completely understand this argument for making 1em everywhere.

@pmetzger

This comment has been minimized.

Copy link
Member

pmetzger commented May 8, 2018

Yeah I completely agree that would make things better. I'm a bit worried that having a bunch of custom fonts will decrease the performance of an otherwise lightweight page which is why I haven't added them in this PR. I think it might take some discussion

Well, hopefully it will not be such a big deal. The fonts will be cached locally and only retrieved at long intervals. That said, I'm in favor of going incrementally on this in general.

One issue is that I don't particularly love Source Code Pro for the monospace font but it is the most likely candidate. I have a fondness for fonts like Courier for this purpose but Courier is not open source.

@dbuenzli

This comment has been minimized.

Copy link
Contributor

dbuenzli commented May 8, 2018

I know 馃槥 Problem is the manual doesn't use <code>, it uses spans with an auto generated class so it's hard to change the inline monospace text safely.

Not sure I get the problem what's wrong with adding a .c003 { font-size: XXX } in the stylesheet ?

@pmetzger

This comment has been minimized.

Copy link
Member

pmetzger commented May 8, 2018

@dbuenzli I've tried doing just that and it still didn't come out right. Play with it yourself, you'll see the problem.

@pmetzger

This comment has been minimized.

Copy link
Member

pmetzger commented May 8, 2018

(It might be some quirk of Chrome vs. other browsers, I didn't try them all, but I think it's a deeper CSS issue that I couldn't quite figure out.)

@dbuenzli

This comment has been minimized.

Copy link
Contributor

dbuenzli commented May 8, 2018

@dbuenzli I've tried doing just that and it still didn't come out right. Play with it yourself, you'll see the problem.

From the chrome dev tools, can't see any problem.

@charlesetc

This comment has been minimized.

Copy link
Contributor Author

charlesetc commented May 8, 2018

I think... I think hevea checks for things that look like that and ignore them? It gets stripped from the css when I do that. I've got a workaround that I'm pushing now...

@pmetzger

This comment has been minimized.

Copy link
Member

pmetzger commented May 8, 2018

Did you edit the CSS or did you set the font size in the chrome console? The latter does work for me. :)

@pmetzger

This comment has been minimized.

Copy link
Member

pmetzger commented May 8, 2018

But really, if this does indeed work, do it, I'm not opposed obviously.

@pmetzger

This comment has been minimized.

Copy link
Member

pmetzger commented May 8, 2018

@charlesetc Ah, maybe hevea was stripping the things. That could be it.

@@ -37,6 +37,12 @@
text-align: center;
}

\newstyle{@media all}{

This comment has been minimized.

@dbuenzli

dbuenzli May 8, 2018

Contributor

Add a comment to explain why you do this. Are you sure it's a stripping issue ? I would rather suspect a CSS order issue but I didn't see the generated css (if that's the case you could try a !important).

This comment has been minimized.

@pmetzger

pmetzger May 8, 2018

Member

It would be neat if, instead, we just had <code> blocks. But whatever works for now.

This comment has been minimized.

@charlesetc

charlesetc May 8, 2018

Author Contributor

Yeah good point. It's definitely something like stripping; putting just \newstyle{.c003}{ font-size: XXX } doesn't result in anything in the generated css

This comment has been minimized.

@charlesetc

charlesetc May 8, 2018

Author Contributor

Then again, targeting generated class names is probably not best practice: what if the number changes?

This comment has been minimized.

@pmetzger

pmetzger May 8, 2018

Member

That has bothered me a lot. I'd like the class names to be meaningful and thus known to be retained. BTW, Hevea is now on Github: https://github.com/maranget/hevea

@jberdine

This comment has been minimized.

Copy link

jberdine commented May 8, 2018

Maybe just me, but the size of the monospace vs sans font here is pretty large:
image

@pmetzger

This comment has been minimized.

Copy link
Member

pmetzger commented May 8, 2018

@jberdine It looks similar to me. But this is the sort of thing which we wanted to know so it can be fixed!

@charlesetc

This comment has been minimized.

Copy link
Contributor Author

charlesetc commented May 8, 2018

@charlesetc

This comment has been minimized.

Copy link
Contributor Author

charlesetc commented May 10, 2018

馃憤 the other option is to host our own monospace font. To get it to match, we'd want to host a font for the body text too. I'm thinking that might be best in another PR, I could set it for 1em for now.

having the page override some but not all of the user-set fonts

The only custom font I've added is in the titles so I don't think that's conflicting with anything. The font family for these code sections is just set to monospace

@charlesetc

This comment has been minimized.

Copy link
Contributor Author

charlesetc commented May 15, 2018

Updated everything with font size 1rem everywhere. Might be better to add more fonts in the future and control their sizes, but this PR's goal is to reset the font size to the normal small one

font-size: 1rem;
max-width: 800px;
width: 85%;
margin: auto;

This comment has been minimized.

@gasche

gasche May 15, 2018

Member

What is the purpose of these changes? The max-width setting here is different from the one in macros.hva, there is a width: 85%;, and then padding turned into margin: auto, what are those doing?

@charlesetc

This comment has been minimized.

Copy link
Contributor Author

charlesetc commented May 15, 2018

So this makes the library reference centered and on small screens 85%, on large screens 800px. With the decrease in font size, I thought this looked better and matched the overall look of the manual more. I didn't do 750px because all the layout in this part is done with tables and has a tendency to overflow. If you or others think it looks better at 750 or spanning the whole page I'm happy to change it.

@charlesetc

This comment has been minimized.

Copy link
Contributor Author

charlesetc commented May 15, 2018

Margin: auto is just a way to center content that has a specified width. It goes along with setting the max width and isn't responsible for any other visual changes.

@gasche

This comment has been minimized.

Copy link
Member

gasche commented May 15, 2018

If you or others think it looks better at 750 or spanning the whole page I'm happy to change it.

No, I'm happy with your choices, but I just wanted to understand why they were part of the PR.

@gasche

This comment has been minimized.

Copy link
Member

gasche commented May 15, 2018

I have the impression that the goal of this PR is met, and on my machine any mismatch in size between code and non-code font is gone. I would like to merge the PR in the next few days, if there is no objection. If you had trouble with previous iterations, feel free to check that it now works for you.

@gasche

gasche approved these changes May 15, 2018

Changes Outdated
@@ -357,6 +357,10 @@ OCaml 4.07

### Manual and documentation:

- PR#1757: change html manual to use relative font sizes

This comment has been minimized.

@gasche

gasche May 15, 2018

Member

The number should be 1767.

This comment has been minimized.

@trefis

trefis May 15, 2018

Contributor

Shouldn't it also be GPR instead of PR?

This comment has been minimized.

@pmetzger

pmetzger May 15, 2018

Member

Yes, it should be GPR, and it should be at the end of the section, not at the start.

This comment has been minimized.

@charlesetc

charlesetc May 15, 2018

Author Contributor

Whoops! Thanks - I also fixed the previous entry I made

@pmetzger

This comment has been minimized.

Copy link
Member

pmetzger commented May 15, 2018

@charlesetc Is there a place we can view what the pages now look like?

@charlesetc charlesetc force-pushed the charlesetc:manual-font-size branch from c914f1d to 17a1059 May 15, 2018

@charlesetc

This comment has been minimized.

Copy link
Contributor Author

charlesetc commented May 15, 2018

Rebased to avoid merge conflict in changes file. @pmetzger preview_docs.narwhal.space should be up to date

@pmetzger

This comment has been minimized.

Copy link
Member

pmetzger commented May 15, 2018

Okay, looked through. I have to say, the body font still looks too small to me. I still think defaulting to 1.1875rem or 1.125rem if people object too much is superior.

@hcarty

This comment has been minimized.

Copy link
Contributor

hcarty commented May 15, 2018

I like the amount of content per line in this iteration. If I want bigger fonts (and I honestly kind of do) I also want them to take up more horizontal space. That's easy enough to get by zooming.

@pmetzger

This comment has been minimized.

Copy link
Member

pmetzger commented May 15, 2018

@hcarty You really don't want them to take up more horizontal space. There are too many characters per line at this point. Humans cannot cope well with lines of more than 60-90 characters, and the bottom of that range is significantly easier to cope with than the top of it. This is a well understood design principle and is very important for readability.

@pmetzger

This comment has been minimized.

Copy link
Member

pmetzger commented May 15, 2018

@hcarty See rule 4 here: https://practicaltypography.com/summary-of-key-rules.html (and explanation of that rule elsewhere in this online book: https://practicaltypography.com/line-length.html )

Note that I think 45 chars (which is what Butterick suggests as a minimum) is too little, 90 too much. Personally I think the sweet spot is around 60 or 65 chars per line average.

@gasche gasche merged commit 493336e into ocaml:trunk May 18, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

gasche added a commit that referenced this pull request May 29, 2018

Merge pull request #1767 from charlesetc/manual-font-size
Change html manual to use relative font sizes

(cherry picked from commit 493336e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.