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

CSS revamp - Improve typography and overall styling #757

Merged
merged 8 commits into from
Jun 20, 2018
Merged

CSS revamp - Improve typography and overall styling #757

merged 8 commits into from
Jun 20, 2018

Conversation

yangshun
Copy link
Contributor

@yangshun yangshun commented Jun 11, 2018

Motivation

I removed a lot of Docusaurus base CSS and use styles from GitHub's markdown styles. Also took the chance to remove some unreferenced CSS and cleaned up some implementations. The site should look nicer now (IMO) 😄

Some changes

  • Improved typography - paddings and margins are more consistent now
  • Removed unnecessary styling in GridBlock
  • Footer is now darker shade of black for better contrast
  • Change background color to #fff
  • Headers are now black by default (from primaryColor) and have bolder font weight
  • Blockquotes are changed to yellow (from pink)
  • Left sidebar is sticky by default on supported browsers

Have you read the Contributing Guidelines on pull requests?

Yes.

Test Plan

Load preview site on laptop and mobile and click on all pages. I did a pretty thorough comparison of the live site and the development site.

Related PRs

NA.

@yangshun yangshun changed the title Clean up CSS Improve typography and CSS Jun 11, 2018
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jun 11, 2018
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Jun 11, 2018

Deploy preview for docusaurus-preview ready!

Built with commit 1883481

https://deploy-preview-757--docusaurus-preview.netlify.com

@JoelMarcey
Copy link
Contributor

Whoa! Looking forward to the Netlify preview.

@yangshun yangshun changed the title Improve typography and CSS Improve typography and overall styling Jun 11, 2018
@endiliey
Copy link
Contributor

I got the chance to use a public pc. I have few comments.

1. Look at the headerNav. On the homepage it's kindof padded but the docs headNav is not. Is this intended ?

Before
before

before2

After
after

after2

2. I kindof like the previous header color. The color depends on the siteConfig.primaryColor
It looks like it's now set to bold & black. WDYT ?

Our docs
siteconfig

Before
before

before2

After
after

after2

@yangshun
Copy link
Contributor Author

yangshun commented Jun 11, 2018

@endiliey Observant you are!

  1. Look at the headerNav. On the homepage it's kindof padded but the docs headNav is not. Is this intended?

Yes. It depends on whether the body is the wide width. The header contents width will match the body.

  1. I kindof like the previous header color. The color depends on the siteConfig.primaryColor
    It looks like it's now set to bold & black. WDYT ?

IMO the current version is more professional. Many documentation sites have black headings. I referenced gatsbyjs.org, graphql.org, reactjs.org, redux.js.org (Gitbook), vuepress.vuejs.org, docsify.js.org, etc and all of them use black/gray for heading color. If users want they can still override the colors themselves but I think it's better for them to not have a colored heading.

@yangshun yangshun changed the title Improve typography and overall styling CSS revamp - Improve typography and overall styling Jun 11, 2018
endiliey added a commit to endiliey/endiliey.github.io that referenced this pull request Jun 11, 2018
Copy link
Contributor

@endiliey endiliey left a comment

Choose a reason for hiding this comment

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

I got the chance to test this out on my test site. Thanks to TravisCI it's as simple as editing package.json. Looks good :smile

@yangshun
Copy link
Contributor Author

@JoelMarcey Can this PR be merged? 😄

@endiliey
Copy link
Contributor

Let's merge. holding my hand

@JoelMarcey
Copy link
Contributor

I want to gauge your opinion on something....

What do you think of gating this change behind a temporary siteConfig option for our forthcoming 1.3 release in the next couple of days (say newUI: true) so that people can opt-in for now? Then when we release 2.0, we can remove the option and everyone has to go to the new CSS.

Copy link
Contributor

@JoelMarcey JoelMarcey left a comment

Choose a reason for hiding this comment

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

I just chatted with @yangshun.

I am going to let @yangshun have the honors of merging this change. He is planning to do a bit more testing. But I am good with merging this when he feels ready.

@JoelMarcey
Copy link
Contributor

@yangshun Hey! What are we thinking with this PR? We should probably cut a release with the fixes we made in the next couple of days. Did we want to add this in? Or cut another release shortly after with it?

@yangshun
Copy link
Contributor Author

Let's leave this out for the next release. I'm still working on stability and testing.

@yangshun yangshun added the status: don't merge yet This PR is completed, but we are still waiting for other things to be ready. label Jun 16, 2018
@ahmadalfy
Copy link
Contributor

Ping me whenever you need ✋

@yangshun
Copy link
Contributor Author

yangshun commented Jun 17, 2018

I think I'm good with this version. I made the sidebar sticky by default, which is the version that Babel and Reason website is doing. I think this should be the default for sites. Please have a go at it! Would be good to help test out on other browsers as well, especially IE. @endiliey @JoelMarcey @ahmadalfy

Netlify link - https://deploy-preview-757--docusaurus-preview.netlify.com/

@JoelMarcey
Copy link
Contributor

@yangshun New 1.2.1 release out and tagged.

I am thinking, when this is merged, it automatically gets us to 1.3 (probably not 2.0, but close). I think for 1.3, we should also look at removing one or both of the lock files too.

@yangshun yangshun merged commit 1094dbd into facebook:master Jun 20, 2018
@yangshun yangshun mentioned this pull request Jun 20, 2018
@JoelMarcey
Copy link
Contributor

@yangshun 🎉

This will be part of a 1.3 release. We will need to document this well in the changelog and probably tweet about it separately too.

@yangshun
Copy link
Contributor Author

@JoelMarcey I'll be releasing 1.3 and writing up the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA status: don't merge yet This PR is completed, but we are still waiting for other things to be ready.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants