Skip to content

Adjust critical CSS extraction and placement #27211

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 7 commits into from
Closed

Adjust critical CSS extraction and placement #27211

wants to merge 7 commits into from

Conversation

krzksz
Copy link
Contributor

@krzksz krzksz commented Mar 9, 2020

Description (*)

This pull requests introduces following changes:

  • Async CSS loading pattern is updated according to the newest best practice, allowing us to completely remove rel="preload" polyfill.
  • Moving existing stylesheets around no longer uses regular expressions which should reduce memory consumption and prevent backtrace limit errors for large pages.
  • Styles are now added once, before </head> tag, after critical CSS.

Related Pull Requests

Fixed Issues (if relevant)

  1. CSS shows up twice when Critical CSS Enabled #26498: CSS shows up twice when Critical CSS Enabled

Manual testing scenarios (*)

  1. Enable critical CSS in Magento's admin panel and flush the case.
  2. Verify that all external stylesheets are loaded asynchronously without blocking first contentful and meaningful paint.
  3. Verify that all external stylesheets are located in the same order at the end of <head>, after inline cricitcal CSS.

Questions or comments

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds are green)

@m2-assistant
Copy link

m2-assistant bot commented Mar 9, 2020

Hi @krzksz. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.4-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

@krzksz krzksz mentioned this pull request Mar 12, 2020
Merged
4 tasks
@ihor-sviziev ihor-sviziev self-assigned this Mar 12, 2020
@ihor-sviziev ihor-sviziev linked an issue Mar 12, 2020 that may be closed by this pull request
CSS shows up twice when Critical CSS Enabled #26498
Closed
@ihor-sviziev
Copy link
Contributor

In general looks good to me. Let’s wait for test results

ihor-sviziev
ihor-sviziev previously approved these changes Mar 13, 2020
View reviewed changes
Copy link
Contributor

@ihor-sviziev ihor-sviziev left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Functional test failures not related to changes from this PR.
SVC failure should be approved, but I think in this case it’s not a big deal
Approved!

@krzksz thank you for amazing job!

@magento-engcom-team
Copy link
Contributor

Hi @ihor-sviziev, thank you for the review.
ENGCOM-7086 has been created to process this Pull Request

@engcom-Delta engcom-Delta self-assigned this Mar 17, 2020
ihor-sviziev
ihor-sviziev previously approved these changes Mar 19, 2020
View reviewed changes
@magento-engcom-team
Copy link
Contributor

Hi @ihor-sviziev, thank you for the review.
ENGCOM-7086 has been created to process this Pull Request

@engcom-Delta
Copy link
Contributor

✔️ QA passed

@engcom-Echo engcom-Echo self-assigned this Mar 25, 2020
@engcom-Echo
Copy link
Contributor

Failed functional tests not related to the changes in this PR

@engcom-Echo
Copy link
Contributor

Backward incompatible changes (deleted block)- need approve

@ghost ghost dismissed ihor-sviziev’s stale review March 27, 2020 14:31

Pull Request state was updated. Re-review required.

@krzksz krzksz requested a review from engcom-Delta April 5, 2020 09:19
ihor-sviziev
ihor-sviziev requested changes Apr 17, 2020
View reviewed changes
Copy link
Contributor

@ihor-sviziev ihor-sviziev left a comment

Choose a reason for hiding this comment

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

Hi @krzksz,
Now we have array to string conversion after your latest change.
Please fix it

@ihor-sviziev ihor-sviziev added the Severity: S3 Affects non-critical data or functionality and does not force users to employ a workaround. label Apr 28, 2020
@VladimirZaets
Copy link
Contributor

Hi @krzksz, do you have any updates?

@krzksz
Copy link
Contributor Author

krzksz commented May 3, 2020

@VladimirZaets Hey, I'm waiting for the conclusion in #27270, then we can apply the final approach here.

ihor-sviziev
ihor-sviziev requested changes May 20, 2020
View reviewed changes
Copy link
Contributor

@ihor-sviziev ihor-sviziev left a comment

Choose a reason for hiding this comment

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

Hi @krzksz,
#27270 was already merged. Please update your PR.

@ihor-sviziev
Copy link
Contributor

@krzksz ,
will you be able to update your PR?

@mrtuvn mrtuvn mentioned this pull request Jun 3, 2020
Merged
4 tasks
@ihor-sviziev
Copy link
Contributor

@magento run all tests

@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Jun 11, 2020 •
edited
Loading

Hi @krzksz,
I'm closing this PR due to inactivity and as we already have better PR #28480

Thank you for your contribution!

@m2-assistant
Copy link

m2-assistant bot commented Jun 11, 2020

Hi @krzksz, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ihor-sviziev ihor-sviziev ihor-sviziev requested changes

@DrewML DrewML Awaiting requested review from DrewML

@adifucan adifucan Awaiting requested review from adifucan

@vzabaznov vzabaznov Awaiting requested review from vzabaznov

@engcom-Delta engcom-Delta Awaiting requested review from engcom-Delta

Assignees

@ihor-sviziev ihor-sviziev

@engcom-Delta engcom-Delta

@engcom-Echo engcom-Echo

Labels
Area: Frontend Auto-Tests: Covered All changes in Pull Request is covered by auto-tests Award: bug fix Award: category of expertise Award: test coverage Component: Theme improvement Partner: creativestyle partners-contribution Pull Request is created by Magento Partner Progress: needs update Release Line: 2.4 Severity: S3 Affects non-critical data or functionality and does not force users to employ a workaround.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CSS shows up twice when Critical CSS Enabled
6 participants
@krzksz @ihor-sviziev @magento-engcom-team @engcom-Delta @engcom-Echo @VladimirZaets