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 styles_min/ CSS from styles/ folder #16154

Merged
merged 1 commit into from Jun 30, 2017
Merged

Conversation

wjordan
Copy link
Contributor

@wjordan wjordan commented Jun 29, 2017

Followup to #15790, refactoring the 'min-styles' work to be more reliable/maintainable moving forward.

  1. Remove all styles from styles/ that were already extracted/added to styles_min/. I did this by manually running cssnano (a postcss plugin) to perform the de-duping. (Note it also performed a small bit of additional backwards-compatible style-minification as well.) This will eliminate style-duplication in our version control and the need to maintain two parallel style-bundles in sync moving forward.

  2. Update the logic in the Pegasus route combine styles from both styles/ and styles_min/ into /style.css. Now that each style exists in only one or the other folder, style.css should still contain the full stylesheet set as before.

  3. Add README.md docs to styles/ and styles_min/ folders explaining the usage of these two folders.

bundle styles_min into style.css
add README to styles_min and styles folders documenting usage.
@wjordan wjordan requested review from breville and Erin007 June 29, 2017 14:34
Copy link
Member

@breville breville left a comment

Choose a reason for hiding this comment

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

One general question: given that our CSS should be cached once it's retrieved, is the big win with this split approach that our first-time load is faster? Or does it make every page load somehow faster too, due to less to parse?

@wjordan
Copy link
Contributor Author

wjordan commented Jun 30, 2017

The goal of the previous PR was to minimize the overall download size (and avoid render-blocking while sending extra requests to fetch external CSS) for the first-time (non-cached) load. Google's recommendation is to inline render-blocking CSS for best performance, and this is only possible using the minimal styles bundle (because the original bundle is much too large to inline). There is indeed a balance between inlining content directly into the HTML page (not cached in the browser, so needs to be downloaded with every request) and using shared external resources that can be browser-cached across pages (only downloaded once but requires an extra request on initial load).

I don't think CSS parsing-time itself is much of a factor in any of this. The reason for splitting the CSS up was more because Hema's approach for removing unused css involved manually testing a specific set pages, and so we have to keep serving the original full CSS bundle to pages that hadn't been manually tested in order to prevent unexpected issues. This PR eliminates duplication between the two bundles for maintainability but shouldn't affect anything else.

If anyone ever has the time to manually dig through our use of the Bootstrap and FontAwesome frameworks and eliminate any CSS not used anywhere on our site, we can get rid of the split and just use one minimal styles bundle everywhere again.

@wjordan
Copy link
Contributor Author

wjordan commented Jun 30, 2017

After merging I'll keep an eye on this PR since there's a decent chance something unexpected could have been subtly affected by these changes.

@wjordan wjordan merged commit 1b74f49 into staging Jun 30, 2017
@davidsbailey davidsbailey deleted the min_styles_refactor branch January 17, 2018 17:57
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.

None yet

2 participants