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

Extract font-face rules and inject into head <styles> element #870

Merged

Conversation

aarongerig
Copy link
Contributor

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.

@aarongerig aarongerig force-pushed the feature/extract-font-face-rules branch from 1f1da9a to df1f0b3 Compare May 25, 2020 14:38
Copy link
Contributor

@oliverklee oliverklee left a 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!

@oliverklee oliverklee added the bug label May 25, 2020
@oliverklee oliverklee added this to the 4.0.0 milestone May 25, 2020
Copy link
Contributor

@oliverklee oliverklee left a 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!

Copy link
Contributor

@JakeQZ JakeQZ left a 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.

src/CssInliner.php Outdated Show resolved Hide resolved
tests/Unit/CssInlinerTest.php Outdated Show resolved Hide resolved
Copy link
Contributor

@JakeQZ JakeQZ left a 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.

@aarongerig
Copy link
Contributor Author

Great – thanks for reviewing @JakeQZ !

Copy link
Contributor

@oliverklee oliverklee left a 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.

tests/Unit/CssInlinerTest.php Outdated Show resolved Hide resolved
Copy link
Contributor

@oliverklee oliverklee left a 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 ! ❤️

@oliverklee oliverklee merged commit a23db03 into MyIntervals:master May 28, 2020
@aarongerig
Copy link
Contributor Author

Awesome, thanks so much guys! 🙏🏻

@aarongerig aarongerig deleted the feature/extract-font-face-rules branch May 28, 2020 08:45
@aarongerig
Copy link
Contributor Author

@oliverklee Are you planning to publish a new release, which includes this feature soon? :)

@oliverklee
Copy link
Contributor

Yes: #874

This pull request was closed.
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.

3 participants