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.

Sign up for GitHub

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

Jump to bottom

Fonts rework #12114

Merged
merged 3 commits into from
Jul 6, 2020
Merged

Fonts rework #12114

merged 3 commits into from
Jul 6, 2020

Conversation

silverwind
Copy link
Member

  • Use system fonts only for text to avoid FOUT
  • Move font-awesome to npm/webpack
  • Move NotoColorEmoji to web_src
  • Remove presumably unneccesary 'PT Sans Narrow'
  • Simplify webpack import exclusions

Fixes: #11818
Fixes: #11814

mrsdizzie
mrsdizzie reviewed Jul 1, 2020
View reviewed changes
web_src/less/_base.less
@@ -125,7 +75,7 @@
.ui.steps .step .title,
.ui.text.container,
.ui.language > .menu > .item& {
font-family: Roboto, @fonts, sans-serif;
Copy link
Member

Choose a reason for hiding this comment

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

Bless 🙏

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 1, 2020
@silverwind silverwind mentioned this pull request Jul 1, 2020
Merged
@lunny lunny added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Jul 2, 2020
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jul 2, 2020
@mrsdizzie
Copy link
Member

LGTM thanks! I'm also of the opinion that we don't really need to ship a emoji fallback webfont and the few people who run systems that don't install one by default and haven't already installed one should expect to just not see them which must already be true for almost every other site they visit.

Feels like an intentional decision for somebody not to have one of these fonts installed at this point and I can't imagine why they would prefer downloading a 9MB Web font instead of installing a real one system wide. I only agreed to it previously because I didn't want the emoji PR held back but think it is unnecessary for us to provide. Maybe for another PR though

@silverwind
Copy link
Member Author

silverwind commented Jul 2, 2020 •
edited
Loading

Yeah I think I tend to agree to remove the emoji fallback but as it stands, it does not hurt us to have. Browsers should only attempt the download if they can't display a certain emoji. Then there's the issue that the font does not work in Firefox because of a bug there, so it's only targeting Chrome/Edge on some Linux distributions really.

@silverwind
Fonts rework
f6ecba5 
- Use system fonts only for text to avoid FOUT
- Move font-awesome to npm/webpack
- Move NotoColorEmoji to web_src
- Remove presumably unneccesary 'PT Sans Narrow'
- Simplify webpack import exclusions

Fixes: go-gitea#11818
Fixes: go-gitea#11814
CirnoT
CirnoT approved these changes Jul 4, 2020
View reviewed changes
Copy link
Contributor

@CirnoT CirnoT 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, would appreciate someone with Chinese locale testing it too however

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jul 4, 2020
@lafriks lafriks added this to the 1.13.0 milestone Jul 6, 2020
@silverwind
Copy link
Member Author

would appreciate someone with Chinese locale testing it too however

I did not change anything about the locale-specific font overrides, so I guess those will be fine.

@lafriks lafriks merged commit 62c2c17 into go-gitea:master Jul 6, 2020
@silverwind silverwind deleted the nativefonts branch July 6, 2020 08:59
@lunny
Copy link
Member

lunny commented Jul 6, 2020

Looks good, would appreciate someone with Chinese locale testing it too however

I will test it after it deployed on try.gitea.io

@CL-Jeremy CL-Jeremy mentioned this pull request Jul 25, 2020
Closed
ydelafollye pushed a commit to ydelafollye/gitea that referenced this pull request Jul 31, 2020
@silverwind @zeripath @lafriks
Fonts rework (go-gitea#12114)
a8b61c6 
- Use system fonts only for text to avoid FOUT
- Move font-awesome to npm/webpack
- Move NotoColorEmoji to web_src
- Remove presumably unneccesary 'PT Sans Narrow'
- Simplify webpack import exclusions

Fixes: go-gitea#11818
Fixes: go-gitea#11814

Co-authored-by: zeripath <art27@cantab.net>
Co-authored-by: Lauris BH <lauris@nix.lv>
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lafriks lafriks lafriks approved these changes

@CirnoT CirnoT CirnoT approved these changes

@mrsdizzie mrsdizzie mrsdizzie approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Milestone
1.13.0
Development

Successfully merging this pull request may close these issues.

Use system-native font stack
7 participants
@silverwind @mrsdizzie @lunny @lafriks @CirnoT @GiteaBot @zeripath