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

core(unused-css): change title Defer -> Remove #7235

Merged
merged 2 commits into from Feb 14, 2019

Conversation

Projects
None yet
4 participants
@patrickhulce
Copy link
Collaborator

patrickhulce commented Feb 13, 2019

Summary
It's a bit confusing that the unused CSS is probably dead CSS that's completely unused. We'll save that distinction for the help text and make the title a little more aggressive.

Related Issues/PRs
closes #6588

@patrickhulce patrickhulce requested a review from exterkamp as a code owner Feb 13, 2019

@exterkamp
Copy link
Member

exterkamp left a comment

LGTM, I +1 this new language, it is a lot more direct what's going on and what we're looking for.

@paulirish paulirish merged commit 3df08f4 into master Feb 14, 2019

7 checks passed

bundlesize Good job! bundle size < maxSize
Details
cla/google All necessary CLAs are signed
codecov/patch Coverage not affected when comparing 638ba4e...ffd9dcb
Details
codecov/project 93.92% remains the same compared to 638ba4e
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
pr title lint PR title is good to go, boss
Details

@paulirish paulirish deleted the unused_css_change branch Feb 14, 2019

@cagross

This comment has been minimized.

Copy link

cagross commented Mar 7, 2019

This is good news--thank you. This should help clear up the the defer vs. remove issue.

But what about the remaining ambiguity on the Learn More page? Specifically, the title refers to 'unused CSS.' But the Learn More page describes 'critical CSS' and 'uncritical CSS.' I was under the impression all three were separate:

Critical CSS: CSS rules used on the page that affect above-the-fold content, which, if removed, could result in noticeable differences in layout/style.

Uncritical CSS: CSS rules used on the page that affect below-the-fold content, or content not viewable on immediate page load (e.g. in a modal window that the user might open on the page)/

Unused CSS: CSS rules that have no chance at all of being used on the page.

If we use these three designations, it sounds like the optimum way to deal with all would be to inline critical CSS, defer uncritical CSS, and remove unused CSS. Is this indeed how PageSpeed Insights suggests we do things? If so, I don't think the "Learn More" page makes that very clear. Or am I misunderstanding the way in which PageSpeed Insights suggests we load the three types of CSS?

Also, maybe this isn't the place to bring this up? If you'd like me to create a new issue, or append it to an existing issue, I can do that.

@patrickhulce

This comment has been minimized.

Copy link
Collaborator Author

patrickhulce commented Mar 7, 2019

You are right @cagross that is the breakdown of the 3 categories of CSS we're recommending. The documentation is currently managed over in another repo which moves a little slower than core. That's where a fix would be done for that issue.

@cagross

This comment has been minimized.

Copy link

cagross commented Mar 8, 2019

@patrickhulce OK thanks for confirming that there are three distinct CSS 'types,' and what I described corresponds to Google's suggested treatment for each. But where would I suggest that the verbiage of the Learn More page be modified to reflect this? Are you saying I should bring that up as in issue in the WebFundamentals repo? Or are you perhaps saying that the WebFundamentals repo needs to first approve this as best practice, then the Lighthouse repo needs to modify the text on the Learn More page accordingly? Or something else? Or is this already in the works, and I need not concern myself too much with it?

@patrickhulce

This comment has been minimized.

Copy link
Collaborator Author

patrickhulce commented Mar 8, 2019

The change itself will take place in that other repo. It's in the backlog to update, but I'll go ahead and re-open the issue this closed to track its progress.

No further action from you necessary :)

@cagross

This comment has been minimized.

Copy link

cagross commented Mar 9, 2019

@patrickhulce OK got it, sounds good. I see that you re-opened the other ticket. I'll monitor progress there. Thanks very much for confirming everything and giving helpful info.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.