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

Added support for multiple background images and CDN #80

Closed
wants to merge 1 commit into from

Conversation

dtbaker
Copy link
Contributor

@dtbaker dtbaker commented Feb 4, 2017

Related to this post:

https://wordpress.org/support/topic/cdn-with-multiple-background-images/#post-8737831

Example of this fix is here: https://verge-blog-demo.themes.dtbaker.net/

Before this fix the #page CSS was this. It only had a single theme-cdn.dtbaker.net URL replacement. The other images were not referenced to the CDN:

.vergeblog_header_overpng #page {
    background-image: 
         url(https://theme-cdn.dtbaker.net/wp-content/themes/verge-blog/images/under-left-leaf.png),
         url(//verge-blog-demo.themes.dtbaker.net/wp-content/themes/verge-blog/images/over-blue-bird.png),
         url(//verge-blog-demo.themes.dtbaker.net/wp-content/themes/verge-blog/images/under-top.png),
         url(//verge-blog-demo.themes.dtbaker.net/wp-content/themes/verge-blog/images/bg-top-grunge.png);
}

After this fix it correctly replaces all images with the CDN url, like this:

.vergeblog_header_overpng #page {
    background-image: 
         url(https://theme-cdn.dtbaker.net/wp-content/themes/verge-blog/images/under-left-leaf.png),
         url(https://theme-cdn.dtbaker.net/wp-content/themes/verge-blog/images/over-blue-bird.png),
         url(https://theme-cdn.dtbaker.net/wp-content/themes/verge-blog/images/under-top.png),
         url(https://theme-cdn.dtbaker.net/wp-content/themes/verge-blog/images/bg-top-grunge.png);

@futtta
Copy link
Owner

futtta commented Feb 4, 2017

thanks @dtbaker ! I'll have a good look at your code and do some testing. Did you have a special reason for not changing the regex?

@dtbaker
Copy link
Contributor Author

dtbaker commented Feb 4, 2017

@futtta because it's 1am here and my brain wasn't ready to jump into that beefy looking regex :) But feel free to do so!

That regex also needs to return one match for each background-image rule, so the final str_replace() on $code works correctly. But we also need to split out the individual url()s from those single background-image rules. So this makes me think it isn't possible without two regex's. The first regex to identify each background-image rule and a second regex to split the rule into individual image components.

Cheers.

@futtta
Copy link
Owner

futtta commented Feb 4, 2017

@zytzagoo what do you think about this PR? my gut tells me we can do this with less changes in the code (i'm a micro-improvement kind-a-guy), but I might be very wrong ...

@dtbaker
Copy link
Contributor Author

dtbaker commented Feb 4, 2017

The other option I started to play with first was a brand new single regex that just searched for url( ... ) components to replace in the entire css code. This was a cheaper regex than also searching for the background part. This would cover both single and multiple background image rules.

This different approach would basically find/replace all url(..) entries in the CSS. Even those that were not prefaced with a background(-image): rule.

Do you think something like that would work better? The single regex would certainly be slightly faster.

@dtbaker
Copy link
Contributor Author

dtbaker commented Feb 4, 2017

Here's the single regex version #81

^^ much less complicated than this one :)

@zytzagoo
Copy link
Collaborator

zytzagoo commented Feb 4, 2017

@futtta yeah, #81 looks way better... tho the regex used might not cover all the edge cases (for example, the spec allows for any number of spaces between url and the opening ( bracket etc.) -- that's just from light reading, too tired to really dig in.

I'm using a similar (but maybe more "complete") regex in my branch here: https://github.com/zytzagoo/autoptimize/blob/tests/classes/autoptimizeStyles.php#L9

@dtbaker dtbaker closed this Feb 5, 2017
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.

3 participants