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

Cssrelpreload #129

Merged
merged 5 commits into from
Jun 16, 2016
Merged

Cssrelpreload #129

merged 5 commits into from
Jun 16, 2016

Conversation

bezoerb
Copy link
Collaborator

@bezoerb bezoerb commented Apr 18, 2016

Bump inline-critical to use loadcss recommended-usage-pattern <link rel="preload">

@addyosmani what do you think? Do we want this in?

@bezoerb
Copy link
Collaborator Author

bezoerb commented May 7, 2016

@addyosmani ping ;)

@@ -142,7 +142,7 @@ function run(data) {
if (err) {
error(err);
} else {
process.stdout.write(val);
process.stdout.write(val, process.exit);
Copy link
Collaborator

@XhmikosR XhmikosR Jun 11, 2016

Choose a reason for hiding this comment

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

Are you sure this change was meant to be included in this PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm sure there was some reason i put it there even if i can't get it right now as this doesn't have anything todo with the PRs main purpose. Do you have any insights on this being a bad practice?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nothing in particular. I just don't like the fact that this change is
unrelated to the rest. You could make a new PR for this and keep things
clean.
On Jun 11, 2016 23:49, "Ben Zörb" notifications@github.com wrote:

In cli.js
#129 (comment):

@@ -142,7 +142,7 @@ function run(data) {
if (err) {
error(err);
} else {

  •            process.stdout.write(val);
    
  •            process.stdout.write(val, process.exit);
    

I'm sure there was some reason i put it there even if i can't get it right
now as this doesn't have anything todo with the PRs main purpose. Do you
have any insights on this being a bad practice?


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/addyosmani/critical/pull/129/files/1d124336fb5352b7c6734f669bb4f713b0a29f66#r66711590,
or mute the thread
https://github.com/notifications/unsubscribe/AAVVtYNsNnoz5U2xeSvk7hUqQxOh-tvdks5qKx9lgaJpZM4IJ0Vx
.

@XhmikosR
Copy link
Collaborator

Maybe a rebase and squash and should be ready :)

@akrawchyk
Copy link

I want this for #142 thanks @bezoerb!

@akrawchyk
Copy link

Actually on second thought, I didn't realize we're expecting the loadCSS to be added to the page with the inline option set. I like this more! I've been adding the rel="preload" links to my HTML manually and running critical over that. Instead this automates it.

@addyosmani
Copy link
Owner

I completely missed the pings on this. Sorry @bezoerb! My GitHub notifications are..out of hand:

screen shot 2016-06-16 at 17 31 12

This LGTM and I like the idea of using version. Let's land this one and close up #142.

@addyosmani addyosmani merged commit 6a2e129 into master Jun 16, 2016
@XhmikosR
Copy link
Collaborator

@addyosmani: irrelevant to this but please check the grunt-uncss notifications too :)

@addyosmani
Copy link
Owner

@XhmikosR will do :)

@sindresorhus sindresorhus deleted the cssrelpreload branch June 17, 2016 00:05
@bezoerb
Copy link
Collaborator Author

bezoerb commented Jul 11, 2016

Thanks @addyosmani and never mind the missing pings ;)

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.

4 participants