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

Added CSS.escape to font-family.ts #4545

Merged
merged 3 commits into from
Nov 18, 2023

Conversation

SanderLeenders
Copy link
Contributor

Please describe your changes

Added CSS.escape to renderHTML. Prevents invalid css when using fonts with numbers in their names, like https://fonts.google.com/specimen/Exo+2

How did you accomplish your changes

Added CSS.escape() to renderHTML

How have you tested your changes

Tested local

How can we verify your changes

Before Screenshot 2023-10-18 at 15 07 03
After
image

Remarks

Checklist

  • The changes are not breaking the editor
  • Added tests where possible
  • Followed the guidelines
  • Fixed linting issues

Related issues

@netlify
Copy link

netlify bot commented Oct 18, 2023

Deploy Preview for tiptap-embed ready!

Name Link
🔨 Latest commit d7f1476
🔍 Latest deploy log https://app.netlify.com/sites/tiptap-embed/deploys/65574a014e5eb7000804b84f
😎 Deploy Preview https://deploy-preview-4545--tiptap-embed.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@janthurau janthurau self-assigned this Nov 17, 2023
Added CSS.escape to renderHTML. Prevents invalid css when using fonts with numbers in their names, like https://fonts.google.com/specimen/Exo+2
@janthurau
Copy link
Collaborator

@SanderLeenders I think this will also escape ,, if you specify multiple fonts, right? So we'd probably need to escape only the real font names. See on the demo:

image

@SanderLeenders
Copy link
Contributor Author

@janthurau you are right! I've added a fix for this.

@janthurau janthurau merged commit 8358a2b into ueberdosis:develop Nov 18, 2023
14 checks passed
@ricardasnav
Copy link

Hi
This change broke rendering on the server side where CSS is not available as a global variable. I use Cloudflare workers to render the output and now it throws.

@nperez0111
Copy link
Contributor

I'm not sure that this was the right move, I think I'm going to make a PR to revert this

@jonahallibone
Copy link

I'm not sure that this was the right move, I think I'm going to make a PR to revert this

This SO answer provides an alternate solution:

https://stackoverflow.com/questions/7638775/do-i-need-to-wrap-quotes-around-font-family-names-in-css

@nperez0111
Copy link
Contributor

I'm not sure that this was the right move, I think I'm going to make a PR to revert this

This SO answer provides an alternate solution:

stackoverflow.com/questions/7638775/do-i-need-to-wrap-quotes-around-font-family-names-in-css

Sorry there are a few recommendations here are you saying that we should wrap with "? That feels hackish.

Or maybe we can do what we did before and just wrap in " if it contains a number or space

nperez0111 added a commit that referenced this pull request May 17, 2024
Using `CSS.escape` is the wrong tool for the job here:
 - it is meant for CSS selectors and does not handle CSS variables properly.
 - you can't use `var(--title)` as a font-family because it was getting escaped to `var\(--title\)`

Instead this introduces using a regex to see if we should quote the font-family. Quoting when:
 - font-family includes at least one number or whitespace character
nperez0111 added a commit that referenced this pull request Jun 4, 2024
Using `CSS.escape` is the wrong tool for the job here:
 - it is meant for CSS selectors and does not handle CSS variables properly.
 - you can't use `var(--title)` as a font-family because it was getting escaped to `var\(--title\)`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants