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

feat: add modern system font stacks #4123

Merged
merged 16 commits into from
Sep 18, 2024
Merged

feat: add modern system font stacks #4123

merged 16 commits into from
Sep 18, 2024

Conversation

kof
Copy link
Member

@kof kof commented Sep 15, 2024

Closes #4121

Description

We discovered this repository of awesome system fonts with fallbacks, well organized.

https://github.com/system-fonts/modern-font-stacks

Assumption here is that most people don't actually need a very specific font rendered, but rather need a type of a font and they are perfectly fine with fallbacks, but without this awesome, tested fallbacks list, how can you know which fallback to use.

With this we will encourage more people to use system fonts.

image

Steps for reproduction

  1. click button
  2. expect xyz

Code Review

  • hi @kof, I need you to do
    • conceptual review (architecture, feature-correctness)
    • detailed review (read every line)
    • test it on preview

Before requesting a review

  • made a self-review
  • added inline comments where things may be not obvious (the "why", not "what")

Before merging

  • tested locally and on preview environment (preview dev login: 5de6)
  • updated test cases document
  • added tests
  • if any new env variables are added, added them to .env file

@kof kof requested review from johnsicili and TrySound September 15, 2024 16:42
@TrySound TrySound changed the title Add modern system font stacks feat: add modern system font stacks Sep 15, 2024
@TrySound
Copy link
Member

Items are not checked on first click, only on second one

Screen.Recording.2024-09-16.at.18.51.01.mov

@johnsicili
Copy link
Contributor

johnsicili commented Sep 16, 2024

  1. I think the tooltips should have zero delay, or like 150ms, here just because they are on the right and there's no chance somebody will know what they are. But not a requirement.
  2. When hovering over System (something that can't be selected), one of the fonts is still highlighted where none should be if you're not hovering over it.
image
  1. I would add a tooltip to Emoji
  2. It's kind of confusing that the first batch of fonts have tooltips and the bottom ones don't. Everything is categorized as "Font Stacks" yet are the bottom ones really stacks? I suppose they have fallbacks but idk
  3. Clicking a font twice add a check. Should do it first time (or never)
check.mp4
  1. Would be cool to show the actual font look in the picker

@johnsicili
Copy link
Contributor

Might be good to use a settings icon instead of upload. These fonts are very useful without having to upload and the icon doesnt communicate that

@kof
Copy link
Member Author

kof commented Sep 16, 2024

When hovering over System (something that can't be selected), one of the fonts is still highlighted where none should be if you're not hovering over it.

The component we use there has been deprecated, its an old behavior, won' be fixingit

@kof
Copy link
Member Author

kof commented Sep 16, 2024

I would add a tooltip to Emoji

There isn't much to say, is three? same for others

@kof
Copy link
Member Author

kof commented Sep 16, 2024

Items are not checked on first click, only on second one

turns out this bug is already in prod

@johnsicili
Copy link
Contributor

I would add a tooltip to Emoji

There isn't much to say, is three? same for others

I was unaware that we need to specify fonts to display emojis. I still am not sure that it is needed. So I guess the tooltip would clarify this :)

@TrySound
Copy link
Member

select still doesn't work on first click

@kof
Copy link
Member Author

kof commented Sep 17, 2024

@johnsicili actually I just realized emoji fonts were not meant to be used standalone as a font, they are meant to be added to any of the font stacks if you want the emojis to use these fonts

https://github.com/system-fonts/modern-font-stacks?tab=readme-ov-file#emoji-support

In theory, we need to add these to every stack basically

@kof
Copy link
Member Author

kof commented Sep 17, 2024

Deleted emoji stack, don't see the need for it, as in the OSs, fallbacks are alredy in-place

@johnsicili
Copy link
Contributor

  1. Tooltip shows up on font upload icon when using font dialog
    https://github.com/user-attachments/assets/60ae9b93-10f6-460e-a4a9-2f58764c2773

  2. All weights show up even if that weight doesnt exist (nbd just pointing out. custom fonts work fine)
    https://github.com/user-attachments/assets/79133d22-8308-440d-abb0-64dacb27e96f

Otherwise good!

Copy link
Contributor

@johnsicili johnsicili left a comment

Choose a reason for hiding this comment

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

done. small stuff just fyi feel free to merge up to you

@kof kof merged commit 30981bd into main Sep 18, 2024
13 of 15 checks passed
@kof kof deleted the font-stacks branch September 18, 2024 19:51
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.

Modern font stacks
3 participants