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

Improvements to CSS #7834

Merged
merged 6 commits into from
May 16, 2021
Merged

Improvements to CSS #7834

merged 6 commits into from
May 16, 2021

Conversation

danielhaim1
Copy link
Contributor

@danielhaim1 danielhaim1 commented Sep 28, 2019

This is a 🔦 documentation change.

Summary

General improvements to the stylesheet used for Jekyll documentations.

  • I've added the correct font features for Lato
  • Added text-rendering option
  • Improved the font- rules (fall backs)
  • Improved order of CSS properties

Tested on multiple pages.

@ashmaroli
Copy link
Member

ashmaroli commented Sep 28, 2019

Hello @danielhaim1, thank you for submitting this PR.
While we don't enforce a linter on the documentation site's CSS, and as such don't have rules in place,
we prefer indenting with just 2 spaces per level instead of 4 spaces or more per level :

indent_size = 2

- Converted 4 indent to 2 indent
- Added `font-display: swap` to FontAwesome
@danielhaim1
Copy link
Contributor Author

  • Changed indenting to 2 instead of 4 spaces
  • Added font-display feature to FontAwesome

@ashmaroli
Copy link
Member

I see a lot of unnecessary changes like changing double-quotes to single-quotes, removal of comments and other whitespace changes.

Please limit the amount of changes to just those that are actually necessary.

I updated `docs/_sass/_fonts.scss`  with improved comments.
The original comments were removed because they were incorrect. Lato (300) is `Lato Light`,  but stated as `lato-300 latin`
@danielhaim1
Copy link
Contributor Author

@ashmaroli thanks for reviewing again.

I updated docs/_sass/_fonts.scss and improved the comments there. The original comments were removed because they were incorrect. Lato (300) is Lato Light, but stated as lato-300 latin

In other cases, I converted inline CSS comments (/* comment */ to // comment) which are better for scss.

In other changes, I've sorted the CSS properties with each selector declaration in a predefined order to be as close as possible to the browser engine.

@ashmaroli
Copy link
Member

You're welcome. Understandably, the amount of changes increases the amount of time for reviewing.

You can help us out by going through the diff and reverting changes to quotes, undoing removal of blank lines at end of file and basically revert all whitespace changes that will keep the diff shorter

@danielhaim1
Copy link
Contributor Author

@ashmaroli reverting the files back to normal, and only added my most critical changes.
spaces are now back to normal, and the file is no longer sorted by property name.

@danielhaim1
Copy link
Contributor Author

Let me know if you'd like me to comb the file again, it saves approx ~300 lines.

@ashmaroli
Copy link
Member

The most noticeable change from this PR is the rendering of numerals:

Current master:

master

This PR in current state:

pull_request

@ashmaroli ashmaroli requested a review from a team September 29, 2019 10:20
Copy link
Contributor Author

@danielhaim1 danielhaim1 left a comment

Choose a reason for hiding this comment

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

Reverted, looks good to me.

@genuinemoses
Copy link
Member

current: master

Yup! The reverted looks good to me as well.

Copy link
Member

@genuinemoses genuinemoses left a comment

Choose a reason for hiding this comment

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

Every change looks good.

@ashmaroli
Copy link
Member

The reverted looks good

@devMoses Um.. I don't see the revert commit here.
The last preview-build on this branch still shows the numerals rendered with lesser vertical height:
https://deploy-preview-7834--jekyllrb.netlify.com/news/2019/08/20/jekyll-4-0-0-released/

Copy link
Member

@DirtyF DirtyF left a comment

Choose a reason for hiding this comment

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

This looks good to me, thanks, and sorry for the late review :)

@DirtyF
Copy link
Member

DirtyF commented May 16, 2021

@jekyll: merge +site

@jekyllbot jekyllbot merged commit e23b85f into jekyll:master May 16, 2021
jekyllbot added a commit that referenced this pull request May 16, 2021
@jekyll jekyll locked and limited conversation to collaborators May 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants