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

Switch to using the "system font stack" #8982

Closed
wants to merge 1 commit into from

Conversation

betatim
Copy link

@betatim betatim commented Jan 12, 2021

This is a first attempt to close #8634 by switching to a system font stack.

Should I put a line break for the list of fonts? I ran make reformat hoping it would sort out my formatting issues but no luck :D

Screenshot 2021-01-12 at 13 44 28

I copied меншого to before the <em> tag to make it easier to compare (that is why it appears twice). I think it now uses the same font.

What should I do next? Are there things that can be cleaned up now that Source Sans Pro isn't in use any more?

Are there any conventions around adding [WIP] to the PR title or marking it as draft for stuff which needs a bit of hand-holding from an experienced person?

@@ -14,7 +14,7 @@

// SETTINGS - FONTS

$base-font-family: "Source Sans Pro", "Helvetica", Arial, sans-serif;
$base-font-family: -apple-system, BlinkMacSystemFont, Segoe UI, Helvetica, Arial, sans-serif, Apple Color Emoji, Segoe UI Emoji;
$code-font: "Source Code Pro", monospace;
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated but I think we also need to consider something like Fira Code here (separate discussion needed)

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Probably. But it's out of the scope of this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please don’t use “BlinkMacSystemFont”; just using “system-ui” is enough in Blink-based browsers.

@webknjaz
Copy link
Member

I think it now uses the same font.

Have you checked this with DevTools? The italics version seems visually smaller which suggests that the font may still be different.

Should I put a line break for the list of fonts? I ran make reformat hoping it would sort out my formatting issues but no luck :D

I don't think it knows how to format CSS. I guess you can change it any way you want as long as it remains readable.

Are there any conventions around adding [WIP] to the PR title or marking it as draft for stuff which needs a bit of hand-holding from an experienced person?

I think you can mark PRs as Draft when you yourself think that it's not ready for review.

P.S. I don't think I currently have the warehouse repo set up on my laptop but I've made this change via DevTools on the production instance. And in my env, it now selects Arial for both normal paragraph text and the emphasized one.

@betatim
Copy link
Author

betatim commented Jan 12, 2021

Devtools for the two bits of text below. I think this means they are using the same font (not quite sure why FA shows up for the normal text)

Screenshot 2021-01-12 at 15 07 52

Screenshot 2021-01-12 at 15 08 06

This is with firefox on OSX

@betatim
Copy link
Author

betatim commented Jan 13, 2021

As far as I can tell this is ready for reviewing. System font stack is used for all normal text (normal == not monospaced).

@webknjaz webknjaz requested review from di, nlhkabu, pradyunsg and a team January 13, 2021 14:25
@di di removed request for a team and pradyunsg January 13, 2021 16:00
@di
Copy link
Member

di commented Jan 13, 2021

Thanks @betatim for taking a look at this!

Are there things that can be cleaned up now that Source Sans Pro isn't in use any more?

Looks like it's still in use by the admin interface CSS as well as some SVG logos and other templates.

I'd be fine with dropping it in all those places and removing our dependency on fonts.google.com for all Warehouse pages (except our 404 page, this should remain).

However I'll leave this for @nlhkabu to approve since ultimately it has the effect of changing the font all of PyPI will render with.

@di di removed their request for review January 13, 2021 16:15
Base automatically changed from master to main January 21, 2021 18:39
@nlhkabu
Copy link
Contributor

nlhkabu commented Apr 24, 2021

On my side, I'd prefer to maintain Source Sans Pro where we can.
Are we able to target this per language (at least for now), as per https://www.w3.org/International/questions/qa-css-lang ?

FYI, support in this font is coming (slowly....) - see:
adobe-fonts/source-sans#87 and
google/fonts#1744

@nlhkabu
Copy link
Contributor

nlhkabu commented Apr 24, 2021

oh, and I should have said - thanks for your work on this @betatim! 🎉

@betatim
Copy link
Author

betatim commented Apr 26, 2021

Are we able to target this per language (at least for now), as per w3.org/International/questions/qa-css-lang ?

Oh wow! I didn't know that this existed. I'll do some reading to find out what the answer is. If someone knows the answer to this question please post it (don't wait for me to find out first :D )

@divbzero
Copy link
Contributor

divbzero commented Apr 1, 2022

We could close this pull request and instead rely on the new version of Source Code Pro as @nlhkabu described on 23 Apr 2021.

The two third-party changes that @nlhkabu referenced are now resolved:

I confirmed today that https://pypi.org/help/#file-size-limit now displays italic ”меншого” using:

  • Chrome 100.0 on macOS 12.3
  • Firefox 98.0 on Ubuntu 20.04 LTS
  • Safari 15.4 on macOS 12.3

@betatim
Copy link
Author

betatim commented Apr 4, 2022

We could close this pull request and instead rely on the new version of Source Code Pro

That would work for me. I'll close this PR. Thanks for following up!

@betatim betatim closed this Apr 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Italic font gets a fallback in Cyrillic texts
6 participants