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: support custom fonts #96

Merged
merged 7 commits into from
Sep 1, 2024
Merged

feat: support custom fonts #96

merged 7 commits into from
Sep 1, 2024

Conversation

rgodha24
Copy link
Contributor

@rgodha24 rgodha24 commented Aug 26, 2024

I added custom fonts support

it supports any latin fonts from fontsource.

because radix-themes doesn't have a combobox, it's just in a giant <Select>. You can just type in the name of the font from fontsource and it searches through the select for you.
we could change it to use this ariakit example, but that's another dep and I wasn't sure if you'd be fine with that.

Copy link

vercel bot commented Aug 26, 2024

@rgodha24 is attempting to deploy a commit to the Tom Labs Team on Vercel.

A member of the Team first needs to authorize it.

Copy link

vercel bot commented Aug 28, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
ogstudio ✅ Ready (Inspect) Visit Preview Sep 1, 2024 7:25am

Copy link
Owner

@QuiiBz QuiiBz left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I'm not quite sure to understand what "Installed fonts" mean, you could provide more details?

We'll need some virtualized list too, opening the font dropdown is currently freezing the tab for ~500ms on a high-end MacBook:
Screenshot 2024-08-28 at 13 52 26

We should also consider lazy-loading the fonts files themselves, instead of triggering +1500 requests to jsdelivr when opening the dropdown.

Typecheck, eslint and some tests are failing, could you take a look at those?

@rgodha24
Copy link
Contributor Author

Yeah so the way I implemented it is that the installed fonts were supposed to be the only ones loaded from the cdn. I completely forgot that virtualization was needed, and was too lazy to check why my computer lagged out while running it.

I think the theoretically correct UI for this would look like this, where the "+" button opens a searchbox with maybe just 5 entries.
image

I'll update the ui and fix the tests/typechecking after this class :)

@rgodha24
Copy link
Contributor Author

output.mp4

this seems better lol

@rgodha24
Copy link
Contributor Author

I kinda hate being that guy, but the CI is working on my machine. I have no clue why its failing on GH actions. I even did a fresh install of my node_modules with the pnpm store cleaned.

@rgodha24 rgodha24 requested a review from QuiiBz August 29, 2024 18:43
@QuiiBz
Copy link
Owner

QuiiBz commented Aug 31, 2024

I kinda hate being that guy, but the CI is working on my machine. I have no clue why its failing on GH actions. I even did a fresh install of my node_modules with the pnpm store cleaned.

Had the same issue on my two PRs this morning, so it's not you. It was working fine 3 months ago so not sure what's going on yet, will take a look.

@QuiiBz
Copy link
Owner

QuiiBz commented Aug 31, 2024

Fixed the issues in #100, you can rebase

@rgodha24
Copy link
Contributor Author

git is hard but I think it should be good now

@QuiiBz QuiiBz changed the title feat: Custom Fonts feat: support custom fonts Sep 1, 2024
@QuiiBz QuiiBz added the feature label Sep 1, 2024
Copy link
Owner

@QuiiBz QuiiBz left a comment

Choose a reason for hiding this comment

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

Pushed a few refactors & design tweaks (less gap, "pre-installed" tag for default fonts):

Screenshot 2024-09-01 at 08 21 52

Thanks a lot for this PR!

@QuiiBz QuiiBz merged commit 008cbe5 into QuiiBz:main Sep 1, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants