-
-
Notifications
You must be signed in to change notification settings - Fork 283
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
Conversation
There was a problem hiding this 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, 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. |
There was a problem hiding this 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.
Realized I accidentally hit "request changes" rather than just a comment as it should have been.
@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:
|
There was a problem hiding this 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.
@dalthviz, could you update the screenshot with your latest changes? Thanks! |
Hi @CAM-Gerlach about your comments:
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 And thanks for the feedback 👍 and @ccordoba12 a preview |
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.
I'm not 100% sure about this either, since the blog isn't exactly the same width as the docs either and is adaptive. |
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. |
A new preview @CAM-Gerlach again thank you for your feedback 👍. Also, something that only until now I realised was the missing |
@dalthviz, things are looking really good! Some comments:
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. |
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—
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.
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. |
A new preview: @CAM-Gerlach about the |
@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. |
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! |
@dalthviz, please also open a new PR in our website repo to add the Docs link to its navbar. |
Just FYI, if you pull the branch then run
Yup, will do now. |
There was a problem hiding this 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.
Pull Request Checklist
Description of Changes
Amiri
,Raleway
).#EE1C24
.Following the issue to resolve.
Issue(s) Resolved
Fixes #19
Preview