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

Local fonts section implementation in React #268

Merged
merged 18 commits into from
Mar 15, 2023
Merged

Conversation

matiasbenedetto
Copy link
Contributor

@matiasbenedetto matiasbenedetto commented Mar 10, 2023

What?

In this PR:

  • Implement the local font asset upload in the React App
  • Add variable fonts preview capabilities
  • Add variable wieght font logic to build the theme.json (doesn't add fontWeight property if the fonts has a a wghtvariable axis.
  • Change the styles of this section to make it look as the rest of the font section as implemented in Fonts outline sidebar #260

Why?

To allow us to implement the user ability to add more configuration options.

How to test:

For classic fonts:

  • Go to the "Add local font" page
  • Add a few fonts a test if everything is working as before.

For variable fonts:

  • Go to the "Add local font" page
  • Upload a variable font (preferably one that implements the wght variable axis).
  • Try the variable font preview options
  • Upload the font

Copy link
Contributor

@jffng jffng left a comment

Choose a reason for hiding this comment

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

My comments are mostly UI related but it's mostly working as expected, thanks!

Change the styles of this section to make it look as the rest of the font section as implemented in #260

So on PR 260 I see this is what adding a Google Font looks like:

Screenshot 2023-03-14 at 10 18 34 AM

On this PR, the local fonts page looks like this:

Screenshot 2023-03-14 at 10 15 01 AM

Should we aim to bring these closer together in this PR? For example, the padding and the layout are different. I think the initial state looks a bit "broken" until you load a font because there is no indication that the right column is reserved for the font preview:

Screenshot 2023-03-14 at 10 26 30 AM

src/local-fonts/index.js Outdated Show resolved Hide resolved
src/local-fonts/upload-font-form.js Show resolved Hide resolved
src/local-fonts/upload-font-form.js Outdated Show resolved Hide resolved
src/local-fonts/upload-font-form.js Outdated Show resolved Hide resolved
src/local-fonts/upload-font-form.js Outdated Show resolved Hide resolved
@matiasbenedetto
Copy link
Contributor Author

I think the initial state looks a bit "broken" until you load a font because there is no indication that the right column is reserved for the font preview

Good point, I added a message for the initial state.
image

When a variable font is loaded it looks like this now:
image

@matiasbenedetto matiasbenedetto requested a review from jffng March 14, 2023 18:08
Copy link
Contributor

@jffng jffng left a comment

Choose a reason for hiding this comment

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

Nice improvements, thanks! I think it's in a spot we can merge and iterate. Pre-approving with a couple notes:

  1. Let's try and bring the Google Fonts and Local Fonts layout closer together in a follow up.
  2. I noticed a security warning when I ran npm install. So maybe running npm audit and fixing the package vulnerabilities would be of good conscience.

@matiasbenedetto matiasbenedetto merged commit df0b8c0 into trunk Mar 15, 2023
@madhusudhand madhusudhand deleted the refactor/localfonts branch March 16, 2023 07:06
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.

2 participants