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

Amend AMP style elements with sourceURL comment for DevTools #1584

Merged
merged 1 commit into from Nov 2, 2018

Conversation

@westonruter
Copy link
Member

@westonruter westonruter commented Nov 1, 2018

When stylesheets include a sourceURL comment at the end then they will be listed in DevTools as sources which can by analyzed for CSS code coverage:

image

This change is important because it allows developers to identify which CSS rules specifically need to be removed. As discussed in #1583, the CSS tree shaker in the plugin is not able to be very sophisticated in how deeply it shakes the tree because it must be done at runtime.

So this relates to #1583, but it re-uses DevTools to do the code coverage instead of having to re-implement the functionality on the PHP side. This is not a replacement, however, because the code coverage does not indicate the specific stylesheet sources for where the unused CSS rules are coming from.

Thanks to @paulirish for the tip.

@westonruter westonruter added this to the v1.0 milestone Nov 1, 2018
@westonruter westonruter changed the base branch from develop to 1.0 Nov 1, 2018
@westonruter westonruter requested a review from kienstra Nov 2, 2018
@kienstra kienstra self-assigned this Nov 2, 2018
@kienstra
Copy link
Contributor

@kienstra kienstra commented Nov 2, 2018

Approved

Hi @westonruter,
Wow, this'll be really helpful for tree-shaking.

Just like your screenshot above, I also saw that the coverage appears for <style amp-custom>:

amp-custom-coverage

@westonruter westonruter merged commit 519b504 into 1.0 Nov 2, 2018
2 checks passed
@westonruter westonruter deleted the add/dev-tools-css-code-coverage-support branch Nov 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants