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

Refactor: Bring Inter and IBM Plex Mono Fonts to Prod / Update Implementation #10131

Merged

Conversation

TylerAPfledderer
Copy link
Contributor

@TylerAPfledderer TylerAPfledderer commented May 4, 2023

Description

This PR does the following for the Inter font:

  • Bring the font to prod via theme config
  • Remove the Fonts component in favor of a static CSS file of the font faces
    • Stores this new file in the static fonts folder
  • Expose the static directory in Storybook to allow visibility of the Inter font

Also adds the IBM Plex Mono font in regular weight for the monospace font family stack

@gatsby-cloud
Copy link

gatsby-cloud bot commented May 4, 2023

❌ ethereum-org-website-dev deploy preview failed

Your build failed. View the build logs.

@gatsby-cloud
Copy link

gatsby-cloud bot commented May 4, 2023

✅ ethereum-org-website-dev deploy preview ready

@nloureiro
Copy link
Contributor

As far as I could text, the new Inter font works really well!!! It improves a lot the readability.

One question:
Did you remove the monospace font family?
it's where I see the main differences. Very small but changes a bit
Screen Shot 2023-05-09 12 29 13 AM

We could keep the same monospace font family to avoid loading another font.
If you don't see that as a more significant load, I can also work on a new monospace font.

@TylerAPfledderer @pettinarip , what do you think?

@TylerAPfledderer
Copy link
Contributor Author

TylerAPfledderer commented May 9, 2023

@nloureiro

Indeed I removed the stack for mono by mistake. I will gladly add it back in.

Here is it's original location

Also, I found this twitter post from the author of Inter with pairing suggestions with a mono typeface.

At the very least, based on this I could put Roboto Mono to the top of the stack?

@nloureiro
Copy link
Contributor

@nloureiro looks like your comment posted twice! 😅

Indeed I removed the stack for mono by mistake. I will gladly add it back in.

Here is it's original location

Also, I found this twitter post from the author of Inter with pairing suggestions with a mono typeface.

At the very least, based on this I could put Roboto Mono to the top of the stack?

But adding a new font will add to the load time? That was the main reason not to add a monospace font to the Design system.

Do you think a new font, like IBM Plex, can be negligible on the total load time?

(I prefer IBM plex to Roboto on pairing with inter)

@TylerAPfledderer
Copy link
Contributor Author

@nloureiro correct, we would want to ensure a font is available in the project here for build.

If I'm not mistaken @pettinarip adding a font file locally should only add somewhere less than a kilobyte? If not way less!

@pettinarip
Copy link
Member

pettinarip commented May 9, 2023

If I'm not mistaken @pettinarip adding a font file locally should only add somewhere less than a kilobyte? If not way less!

Not sure if I follow you on that. It will depend on how many fonts we want to add and the size of each one of them.

https://github.com/IBM/plex/releases/tag/v6.3.0 this is the font we want to add, right? https://www.ibm.com/plex/

Questions @nloureiro:

  • Do you think we need to support more than just latin characters?
  • Do you think we need to support more than the regular weight fonts?

If we are only going to support regular-latin chars, here are the fonts to include:

  • IBMPlexMono-Regular-Latin1.woff2 - 16.3kb
  • IBMPlexMono-Regular-Latin2.woff2 - 13.3k
  • IBMPlexMono-Regular-Latin3.woff2 - 6.6kb

Note: not sure the difference between 1, 2, and 3. Perhaps we could just use the Latin1 and that could be enough, not sure.

@nloureiro
Copy link
Contributor

If I'm not mistaken @pettinarip adding a font file locally should only add somewhere less than a kilobyte? If not way less!

Not sure if I follow you on that. It will depend on how many fonts we want to add and the size of each one of them.

https://github.com/IBM/plex/releases/tag/v6.3.0 this is the font we want to add, right? https://www.ibm.com/plex/

Questions @nloureiro:

  • Do you think we need to support more than just latin characters?
  • Do you think we need to support more than the regular weight fonts?

If we are only going to support regular-latin chars, here are the fonts to include:

  • IBMPlexMono-Regular-Latin1.woff2 - 16.3kb
  • IBMPlexMono-Regular-Latin2.woff2 - 13.3k
  • IBMPlexMono-Regular-Latin3.woff2 - 6.6kb

Note: not sure the difference between 1, 2, and 3. Perhaps we could just use the Latin1 and that could be enough, not sure.

Just Regular is fine. We don´t use any other weight on monospace.
Let me see if we can find the difference in these charsets, but I would guess that Latin1 might be enough.

@pettinarip
Copy link
Member

Cool! so, lets wait for @nloureiro's confirmation on what font we need to load and we can add them @TylerAPfledderer

@nloureiro
Copy link
Contributor

Cool! so, lets wait for @nloureiro's confirmation on what font we need to load and we can add them @TylerAPfledderer

on my side I did some research on monospace fonts and I my take is to go with IBM plex mono regular.

we just need regular and the .woff it's 39kb and it's acceptable for the overall loadtime
Screen Shot 2023-05-10 10 49 24 AM

It's going to be used mostly on code snippets so we can use only the Latin set.

on the IBM plex github they recommend this
font-family: 'IBM Plex Mono', 'Menlo', 'DejaVu Sans Mono', 'Bitstream Vera Sans Mono', Courier, monospace;

I think we can just go with
font-family: 'IBM Plex Mono', Courier, monospace;

should be good for us and less code.

my question:
where do we use monospace beside code?

I saw on the homepage
Screen Shot 2023-05-10 10 54 03 AM

are there more places? that I did not found...

@pettinarip
Copy link
Member

Sounds good @nloureiro!

Other parts where we are using the font are:

@nloureiro
Copy link
Contributor

Sounds good @nloureiro!

Other parts where we are using the font are:

Interesting, all these sections that now have the monospace font, I already changed on the Design system.

that being said, I don´t see any change in these examples on the deploy preview, I assume we are using other Font family calls for these examples
Screenshot 2023-05-10 at 15 13 45

Can you check where we are using it?

@pettinarip
Copy link
Member

pettinarip commented May 10, 2023

@nloureiro hmm on the preview deploy for this PR, I don't see the old font definitions, I see this:
image

Can you double-check that you are using the correct preview deploy link?

@nloureiro
Copy link
Contributor

@nloureiro hmm on the preview deploy for this PR, I don't see the old font definitions, I see this: image

Can you double-check that you are using the correct preview deploy link?

yes, it's on the preview deploy.

this is the lines that are supposed to be removed. That's why I think we declare the monospace font in other files
Screenshot 2023-05-10 at 23 22 44

@TylerAPfledderer
Copy link
Contributor Author

@pettinarip @nloureiro can never forget that there is the old theme file that also contains the stack, and might (I guess only sometimes) rear its head! 😅

@nloureiro
Copy link
Contributor

@pettinarip @nloureiro can never forget that there is the old theme file that also contains the stack, and might (I guess only sometimes) rear its head! 😅

Right! That might be it!!!!

In this case, I would say that we can move forward in adding the IBM Plex mono as a monospace font.

I've added to the design system typography section
Screen Shot 2023-05-10 05 31 57 PM
Screen Shot 2023-05-11 04 04 35 PM

@pettinarip do you think it's better to do it on this PR? or create a new one on top of this one?

My take it's that once re touch the monospace family here, we might as well use this to add this font and QA all at the same time

@TylerAPfledderer
Copy link
Contributor Author

@nloureiro @pettinarip I'll at least push a commit to revert the original stack back into the Chakra config

"system-ui, -apple-system, BlinkMacSystemFont, Segoe UI, Roboto, Helvetica, Arial, sans-serif",
body: "system-ui, -apple-system, BlinkMacSystemFont, Segoe UI, Roboto, Helvetica, Arial, sans-serif",
monospace:
"SFMono-Regular, Consolas, 'Roboto Mono', 'Droid Sans Mono', 'Liberation Mono', Menlo, Courier, monospace",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should keep the monospace font family

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, agree
Thank you

@pettinarip
Copy link
Member

My take it's that once re touch the monospace family here, we might as well use this to add this font and QA all at the same time

Agree. Lets add it now and test all the font changes in this PR. @TylerAPfledderer how do you see doing this?

@TylerAPfledderer
Copy link
Contributor Author

TylerAPfledderer commented May 12, 2023

My take it's that once re touch the monospace family here, we might as well use this to add this font and QA all at the same time

Agree. Lets add it now and test all the font changes in this PR. @TylerAPfledderer how do you see doing this?

@pettinarip I would include the new font the same as with Inter, having a CSS file in the ./static directory to expose it to Storybook, or one consolidated CSS file of all the applied @font-face's.

Are you asking about testing as well?

@nloureiro
Copy link
Contributor

I think he is referring to add the new font to this PR, and I will Re-test all the pages to see if all is good.

right?

@pettinarip
Copy link
Member

Correct @nloureiro and @TylerAPfledderer.

Lets add the IBM font in this PR and then, Nuno and I, re-test everything again with the two fonts.

@pettinarip I would include the new font the same as with Inter, having a CSS file in the ./static directory to expose it to Storybook, or one consolidated CSS file of all the applied @font-face's.

I think it will be best to have it in its own file, static/fonts/ibm-plex-font-face.css + a new entry in the preload scripts of gatsby-ssr

@TylerAPfledderer
Copy link
Contributor Author

I think it will be best to have it in its own file, static/fonts/ibm-plex-font-face.css + a new entry in the preload scripts of gatsby-ssr

@pettinarip on it!

@TylerAPfledderer TylerAPfledderer changed the title Refactor: Bring Inter Font to Prod / Update Implementation Refactor: Bring Inter and IBM Plex Mono Fonts to Prod / Update Implementation May 12, 2023
@TylerAPfledderer
Copy link
Contributor Author

@nloureiro @pettinarip this should be good to check with the new font!

Copy link
Contributor

@nloureiro nloureiro left a comment

Choose a reason for hiding this comment

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

I've checked a lot of pages in various OSs and all looks good!

Copy link
Member

@pettinarip pettinarip left a comment

Choose a reason for hiding this comment

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

Wohooo! lgtm thanks @TylerAPfledderer

cc @nloureiro merging this

@pettinarip pettinarip merged commit eb258ff into ethereum:dev May 25, 2023
5 of 6 checks passed
@TylerAPfledderer TylerAPfledderer deleted the refactor/inter-font-through-css branch May 25, 2023 19:46
This was referenced May 31, 2023
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.

None yet

3 participants