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

use extractCritical to set css-vars inline #159

Closed
wants to merge 2 commits into from

Conversation

kylpo
Copy link

@kylpo kylpo commented Jul 17, 2017

What:
This PR adds css-vars to extractCritical so their initial values may be set inline (when in extract mode). Discussed in #147.

How:
Started with @mitchellhamilton's regex substitute, but needed to account for vars- strings a bit differently. Since the regex was greedy, for a string like

.vars-176qc2r {--css-Home-1wjhkv7-0: 0; --css-Home-1wjhkv7-1: 1}

, the matcher would grab a css- string instead of the desired vars-. We could change our regex to be lazy with something like /(?:css|vars)(?:[a-zA-Z0-9-]*?)-([a-zA-Z0-9]+?)/gm, but that seems brittle. Surely there'll be a time when css- will be shorter than the vars-.

So, this implementation checks for vars- first, then moves on to the normal css- check.

Checklist:

  • Documentation
  • Tests
  • Code complete

@codecov-io
Copy link

codecov-io commented Jul 17, 2017

Codecov Report

Merging #159 into master will decrease coverage by 0.08%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #159      +/-   ##
==========================================
- Coverage   90.22%   90.14%   -0.09%     
==========================================
  Files          22       22              
  Lines         706      710       +4     
  Branches      166      168       +2     
==========================================
+ Hits          637      640       +3     
  Misses         56       56              
- Partials       13       14       +1
Impacted Files Coverage Δ
src/server.js 95% <85.71%> (-5%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0fc8909...afdc5c4. Read the comment docs.

@tkh44 tkh44 requested a review from emmatown July 19, 2017 23:19
Copy link
Member

@emmatown emmatown left a comment

Choose a reason for hiding this comment

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

Could you add some tests? You'll want to add them in the test/extract folder.

@kylpo
Copy link
Author

kylpo commented Jul 20, 2017

Happy to! Thanks for pointing me to the correct file.

I'm on vacation until Monday. When I get back, I'll work on it.

@kylpo
Copy link
Author

kylpo commented Aug 1, 2017

Hey, sorry. I haven't forgotten this. Just busy.

@tkh44
Copy link
Member

tkh44 commented Aug 1, 2017

We aren't using vars in v7 so further work on this shouldn't be necessary.

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.

4 participants