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

Above the fold? #286

Open
peterbe opened this issue Dec 28, 2018 · 11 comments
Open

Above the fold? #286

peterbe opened this issue Dec 28, 2018 · 11 comments

Comments

@peterbe
Copy link
Owner

peterbe commented Dec 28, 2018

Worth seeing if it's worth it. I certainly know I have some examples where the CSS below the fold adds up a fair portion of the total minimal CSS.

The implementation can be something like...

document.querySelector(selector).getBoundingClientRect().top <= MAX_TOP
@muralikg
Copy link

Another way to do this would be to use IntersectionObserver and find everything in the viewport

var observer = new IntersectionObserver(function(entries, observer) {
	entries.forEach(entry=>{
		if (entry.isIntersecting) {
			console.log(entry.target.classList)
		}
	})
});

document.querySelectorAll('*').forEach(element=>{
	observer.observe(element)
})

@muralikg
Copy link

muralikg commented May 22, 2019

My bad I just realised that selectors can be more complex than just a class name.

EDIT
Each element has to be then matched with css selectors

entry.target.matches(cssSelector)

@leeoniya
Copy link

check this project out: https://github.com/ABVanton200/critical-css-parser

@peterbe
Copy link
Owner Author

peterbe commented May 30, 2019

@leeoniya What does that have to do with "Above the fold?"? I know you built DropCSS and you believe it's better than minimalcss but please don't drop random plugs in random places.

@leeoniya
Copy link

What does that have to do with "Above the fold?"

wait, what? that's precisely what this project does: "Get critical (above-the-fold) and rest CSS using Puppeteer". i'm not sure how this is plugging DropCSS. i discovered critical-css-parser via Github's new "Used By" button a few days ago and thought it was interesting. it works by removing elements with a bounding box below the fold. then running a css purger. yes, it happens to use DropCSS but that's besides the point, it could use minimalcss just the same. very confused by your comment here.

@peterbe
Copy link
Owner Author

peterbe commented May 30, 2019

Oh I see. I the above-the-folder of critical-css-parser is implicit, not a specific section. Sorry for jumping to conclusions.
The relevant implementation is this: https://github.com/ABVanton200/critical-css-parser/blob/6945428a05a6f8749804814a71afc5a8ccc16b37/src/index.js#L137

@akashdsouza
Copy link

This is really a much needed feature. But, as it is right now, I'm not sure if this can be supported. minimalcss uses cheerio to check if a selector is present in the dom or not. From my understanding, cheerio does not yet have a method for getBoundingClientRect as it doesn't actually render the page.

One way could be to render the page again in puppeteer to check if it is in viewport. But, this doesn't seem efficient. Any thoughts on how to implement this better?

@peterbe
Copy link
Owner Author

peterbe commented Jun 24, 2019

This is really a much needed feature.

Interesting. What's your use case for that argument?

I did some very rough estimates of my own (months ago) and found it was not worth it. Of the remaining actual critical CSS, almost all of it was needed for the above-the-fold content as so much of that CSS wasn't exclusively for the above-the-fold but for structure and stuff that's needed by the whole page.
From a 100KB thatbloatedcssframework down to 10KB for the critical, and of that maybe 1KB only applicable below the fold. So it's not 0 but the advantage for no bothering is that users can potentially scroll down faster than the page has a chance to load ALL css (e.g. with loadCSS). This way there's no rush to load all CSS which can have performance benefits.

@akashdsouza
Copy link

Ideally, the css for below the fold content may not contribute as much to the page. But, it is usual for styles in common sections as footers to be shared across multiple pages(which wouldn't need to be included). It is also not uncommon to have some css heavy animations down the page.

Despite inlining critical css, we still need to asynchronously load the required stylesheets. This implies all styles will be applied twice on each element.

Essentially, while not a common requirement, it would be better to have an option to only inline above the fold css to get the performance benefits when required in larger pages.

@akashdsouza
Copy link

akashdsouza commented Jun 26, 2019

One way could be to render the page again in puppeteer to check if it is in viewport.

If we leveraged puppeteer's coverage API to extract the selectors, then it would be possible to check if the selector is applied on an element within the view port while the page is open in puppeteer itself.

@akashdsouza
Copy link

akashdsouza commented Jul 1, 2019

I was checking out puppeteer's coverage API. While it is extracts out the used css, it also doesn't include keyframes and font-faces. Further, puppeteer's selector APIs are asynchronous making it difficult to use them with AST parsers like css-tree.

I think it might be better to do something like extracting out above the fold html similar to critical-css-parser as you mentioned here.

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

No branches or pull requests

4 participants