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

PR: Consistent look and feel with the rest of the website #21

Merged
merged 9 commits into from
May 2, 2018

Conversation

dalthviz
Copy link
Member

@dalthviz dalthviz commented Apr 21, 2018

Pull Request Checklist

  • Read and followed this repo's Contributing Guidelines
  • Based your PR on the latest version of the correct branch (master or 3.x)
  • Checked your writing carefully for correct English spelling, grammar, etc
  • Described your changes and the motivation for them below
  • Noted what issue(s) this pull request resolves, creating one if needed

Description of Changes

  • Change fonts (Amiri, Raleway).
  • Change color of links #EE1C24.
  • Add banner/navbar.

Following the issue to resolve.

Issue(s) Resolved

Fixes #19

Preview

imagen

Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

Raleway, the sans body font, matches the website and looks really good, but there is no serif font (much less anything like Amiri) anywhere I can see on the website (much less the headings) and its a bit of an odd contrast with the otherwise very modern look of the docs and particularly the website. It seems a similar/identical sans font (Raleway or the like) is actually what is used for the headings on the site, and thus presumably should be the case here (and would look better), particularly if the goal is to match the look and feel of the former. I believe Amiri may have been used initially a week or two ago, but it was changed some time ago; IIRC I suggested something to that effect at the time.

@ccordoba12
Copy link
Member

CAM, Amiri is currently used in the headings of our blog. I think it looks really good there, but not in the front page. That's why I removed it.

Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

Raleway, the sans body font, matches the website and looks really good, but there is no serif font (much less anything like Amiri) anywhere I can see on the website (much less the headings) and its a bit of an odd contrast with the otherwise very modern look of the docs and particularly the website. It seems a similar/identical sans font (Raleway or the like) is actually what is used for the headings on the site, and thus presumably should be the case here (and would look better), particularly if the goal is to match the look and feel of the former. I believe Amiri may have been used initially a week or two ago, but it was changed some time ago; IIRC I suggested something to that effect at the time.

@CAM-Gerlach CAM-Gerlach dismissed their stale review April 21, 2018 18:38

Realized I accidentally hit "request changes" rather than just a comment as it should have been.

@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented Apr 21, 2018

@ccordoba12 Ah, I see—I didn't think to check the blog separately, sorry. Thanks for pointing that out.

In any case, I guess it works okay on the blog, but I would greatly prefer the sans font from the mainpage for the docs for several additional reasons:

  • A serif font like that one communicates the feel of being stuffy, staid and out of date rather than clean, modern and approachable as our documentation should be.
  • It looks much more consistent with our main page, as you pointed out
  • Looking at peer examples, every flagship project I could find—numpy, scipy, jupyter notebook/jupyterlab, matplotlib, IPython, pandas, and particularly sphinx itself all uses sans fonts for headers, the latter even a very similar one to our mainpage
  • It introduces another font into a page that already has at least two (the text and the logo), along with a number of different faces/styles, increasing visual clutter and decreasing coherency
  • From a personal taste point of view, it just looks better and cleaner overall, while the serif one looks somewhat out of place IMO

@dalthviz dalthviz changed the title PR: [WIP] Consistent look and feel with the rest of the website PR: Consistent look and feel with the rest of the website Apr 24, 2018
Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

Banner looks good, except for that its a little bit bigger than the blog and main site (and than it needs to be) in height and font size. I assume you'll backport the small caps to the other two? Also, should we try to match the width of the banner (between the left margin of the logo and the rightmost "docs" text) with the width of the docs and blog pages, so they fit better? Or is keeping perfect continuity with the main page more important?

I reiterate my serious concerns about the serif font for the headings for the above reasons, and my strong preference for Raleway (or similar sans) as the header font as well; it just looks really incoherent as is with all the different fonts and styles going on, and the serif font looks quite out of place with the rest.

I also have the following specific feedback:

  • The body and header font size is way too big for docs; should be at least two point sizes smaller if not more, and should match the sidebar. Especially with the narrow-ish margins, hardly any content fits on a page; its fine for our main site but rather out of place for educational docs.
  • Remove the now-uneeded line break in "The Scientific Python Development Environment"
  • If we're going to link to the main page with the icon/name in the banner, the sidebar logo is essentially redundant and can be removed.

@ccordoba12
Copy link
Member

@dalthviz, could you update the screenshot with your latest changes? Thanks!

@dalthviz
Copy link
Member Author

Hi @CAM-Gerlach about your comments:

Banner looks good, except for that its a little bit bigger than the blog and main site (and than it needs to be) in height and font size. I assume you'll backport the small caps to the other two? Also, should we try to match the width of the banner (between the left margin of the logo and the rightmost "docs" text) with the width of the docs and blog pages, so they fit better? Or is keeping perfect continuity with the main page more important?

I'm not quite sure to see the differences in height and font-size between the docs banner and the banner in the blog or main page (for all is set to 13px the font-size I think). So could you take some screen shots to see the differences. Also yep, I will do the cap of the S in the banners of the other pages. About the width between the ends of the banner (logo and docs), I'm not sure, what do you think @ccordoba12 ?

And thanks for the feedback 👍

and @ccordoba12 a preview
imagen

@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented Apr 25, 2018

I'm not quite sure to see the differences in height and font-size

You are indeed quite right; there is no different in height—I'm not sure what I was thinking, sorry. The difference in the apparent font size is due to the existing word mark being "lowercase" small caps, while the new one on the docs has the full height uppercase "S", making the apparent font size larger. Once you backport that change, they should presumably be identical.

About the width between the ends of the banner (logo and docs), I'm not sure

I'm not 100% sure about this either, since the blog isn't exactly the same width as the docs either and is adaptive.

@dalthviz
Copy link
Member Author

dalthviz commented May 1, 2018

A preview:

imagen

imagen

@CAM-Gerlach
Copy link
Member

Thanks @dalthviz . The font size looks much better now, IMO, and the other changes look good. You can probably just remove the logo from the sidebar entirely since it is basically redundant with the top bar logo/text just above it, and also makes changing the width later more annoying. Of course, I'm still a strong -1 on the serif font for headings instead of using the existing sans like the mainpage, but that's awaiting @ccordoba12 's opinion.

@dalthviz
Copy link
Member Author

dalthviz commented May 1, 2018

A new preview

imagen

@CAM-Gerlach again thank you for your feedback 👍. Also, something that only until now I realised was the missing Fork me on Github link. I added it again :)

@ccordoba12
Copy link
Member

@dalthviz, things are looking really good! Some comments:

  1. Please remove again the Fork me on Github label. It looks odd.
  2. Remove the Star count and the build badge present on top of the table of contents. It makes little sense to show them as part of our docs.

After those changes, I think this one would be ready.

@CAM-Gerlach, let us play for a bit with the Amiri font. I think it looks really well on headers, but we can remove it later if it doesn't fit with our docs.

@dalthviz
Copy link
Member Author

dalthviz commented May 1, 2018

A new preview:

imagen

@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented May 1, 2018

Thanks @dalthviz , looking very good indeed. A few final tweaks, if possible, mostly to reduce the proliferation of font sizes leading to a less coherent and easy to follow look, and to fix a regression/issue this PR introduced—

  • Can you set the code font back to an actual monospaced face, like Consolas or a similar free alternative? I noticed that code now appears in a standard serif font instead after your changes in this PR, which can be considered a regression.
  • Can you make the Navigation header the same font size as the main headings to its right, and vertically aligned with them? This not only leads to a more consistent look, but also fixes the existing awkward look where the doc itself begins before the sidebar.
  • Can you bump the text size up just a bit (from 14 to 15.4px) to match the font size of the subitems in the sidebar, so it is easier to read and for greater style consistency?
  • As a related issue for the website, would you consider making blog text and headers smaller as well so they harmonize with those here (and they are rather big as is)? We don't want the body font to be too small there, but if 15.4 I suggest above looks too small (current size there is 18 px), we can match the 16.8 of the top-level doc navigation links instead with both. Either way, we want to pick something consistent and roll with it, if we want a coherent look.

Some comments:

Agreed with both @ccordoba12 ; I just enabled them in that PR just to show off some of the features of the theme for consideration, and they ended up just getting committed as is.

let us play for a bit with the Amiri font.

After living with it a while, it actually doesn't look too by itself; not my favorite look but I can see how someone might prefer it. My main concern is that it makes the docs look rather out of place with the look of the main website (ala the purpose of this PR), though that can potentially at least be somewhat addressed with the suggested tweaks above to make the look a little more coherent and with less distracting size, formatting and fonts going on.

@dalthviz
Copy link
Member Author

dalthviz commented May 2, 2018

A new preview:

imagen

imagen

@CAM-Gerlach about the font-size I think that definitely we have to decide the standard size (in comparison with the current size here, the size in the blog now looks kind of big). What do you think @ccordoba12 ?

@CAM-Gerlach
Copy link
Member

@dalthviz Fantastic, thanks. Not to keep bugging you, but the Navigation still isn't exactly the correct size; right now its 33.6px while the main heads are 37px.

Now that I look more at it, IMO probably the best solution is to bump the doc font up one more size to 16.8px, and bump the blog down one to that as well. It should be a good compromise between the two, and will also match the sidebar main font size as well. Accordingly, the top/main and sub heads of the blog could also be brought down to 37px and 27.7px from 48px and 39.6px respectively to match as well, as they are quite big as is, although both could be brought to a size a little bigger than the current docs if the latter is a bit too small for all purposes.

@ccordoba12
Copy link
Member

I think that definitely we have to decide the standard size (in comparison with the current size here, the size in the blog now looks kind of big). What do you think @ccordoba12 ?

I'd say we should merge this one so we can take a look at both pages at the same time.

@CAM-Gerlach, could you approve so we can keep iterating? Thanks!

@ccordoba12
Copy link
Member

@dalthviz, please also open a new PR in our website repo to add the Docs link to its navbar.

@CAM-Gerlach
Copy link
Member

I'd say we should merge this one so we can take a look at both pages at the same time.

Just FYI, if you pull the branch then run make html from the doc directory, you can open it in a web browser and it'll look exactly like as if it was deployed live.

@CAM-Gerlach, could you approve so we can keep iterating? Thanks!

Yup, will do now.

Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

Looks good for now; thanks @dalthviz ! I'll merge this so we can continue iterating.

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

Successfully merging this pull request may close these issues.

3 participants