-
Notifications
You must be signed in to change notification settings - Fork 154
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
Extract font-face rules and inject into head <styles> element #870
Extract font-face rules and inject into head <styles> element #870
Conversation
1f1da9a
to
df1f0b3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add tests that fail without the change and pass with it? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And could you also please add a line to the changelog (newest on top)? Thanks!
… font-family and src descriptors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks very good to me - my only (very minor) quibble is with a variable name containing both "or" and "and", which doesn't read quite right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM though I'll let @oliverklee give it a final once over before committing. Thanks @aarongerig for contributing.
Great – thanks for reviewing @JakeQZ ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beautiful! Only one nitpick.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beautiful! Thanks for contributing, @aarongerig ! ❤️
Awesome, thanks so much guys! 🙏🏻 |
@oliverklee Are you planning to publish a new release, which includes this feature soon? :) |
Yes: #874 |
This is the follow up to issue #869.
The code was tested on one of my current projects and there all
@font-face
rules were successfully extracted and injected into the<styles>
element.Let me know if I can do something else to get this into master.