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

Introduce AMP_WP_Styles for doing enqueued scripts inline #887

Merged
merged 4 commits into from Jan 23, 2018

Conversation

Projects
None yet
2 participants
@westonruter
Copy link
Member

commented Jan 20, 2018

This makes enqueueing CSS much more seamless. The only requirement is that you enqueue CSS that exists on the filesystem in a theme/plugin, wp-admin, or wp-includes. It also handles added inline styles, locale stylesheets, RTL styles, and Custom CSS.

The AMP_WP_Styles can then be used to house the logic for “tree shaking” the CSS rules as well. For tree-shaking, we'd need to add a basic CSS tokenizer-parser, with a method that takes the $dom object from #875 for the buffer that is output, and then this can be used to grab all of the class attributes; or it could just use a regexp to grab all of the class="(.+?)" from the output buffer.

Fixes #886.

@ThierryA
Copy link
Collaborator

left a comment

I love this new wp enqueue support! I left my CR below.

*/
public static function override_wp_styles() {
global $wp_styles;
$wp_styles = new AMP_WP_Styles(); // WPCS: global override ok.

This comment has been minimized.

Copy link
@ThierryA

ThierryA Jan 22, 2018

Collaborator

I think it would be safer to prevent the class from being initiated multiple times given that this method is public static. It would be helpful to return the styles too, for testibily for example.

global $wp_styles;
if ( ! ( $wp_styles instanceof AMP_WP_Styles ) ) {
	$wp_styles = new AMP_WP_Styles();
}
return $wp_styles;
* @return string|WP_Error Style's absolute validated filesystem path, or WP_Error when error.
*/
public function get_validated_css_file_path( $src, $handle ) {
if ( ! is_bool( $src ) && ! preg_match( '|^(https?:)?//|', $src ) && ! ( $this->content_url && 0 === strpos( $src, $this->content_url ) ) ) {

This comment has been minimized.

Copy link
@ThierryA

ThierryA Jan 22, 2018

Collaborator

Would be a bit more elegant to have the conditions with line breaks assigned to a variable.

This comment has been minimized.

// Strip query and fragment from URL.
$src = preg_replace( ':[\?#].+:', '', $src );
$src = esc_url_raw( $src );

This comment has been minimized.

Copy link
@ThierryA

ThierryA Jan 22, 2018

Collaborator

Remove empty line above.

* @since 0.7
* @var bool
*/
protected $did_locale_stylesheet = false;

This comment has been minimized.

Copy link
@ThierryA

ThierryA Jan 22, 2018

Collaborator

I personally prefer keeping all variables declarations above the methods, specifically since it is the case in all the other plugin's classes.

PS: this comment applies here too.

$content_url = content_url( '/' );
$admin_url = get_admin_url( null, '/' );
$css_path = null;
if ( 0 === strpos( $src, $content_url ) ) {

This comment has been minimized.

Copy link
@ThierryA

ThierryA Jan 22, 2018

Collaborator

What if the assets are in root folder? For instance could use the filesystem to create dir and cache all CSS which is then enqueued. Here is a function which converts url to path, has be perfected over the years to fix all bugs reported on all sorts of various WP sites structure. Here are all the Unit Tests for it.

This comment has been minimized.

Copy link
@westonruter

westonruter Jan 22, 2018

Author Member

No assets from core, themes or plugins would be in the root folder. So I don't think we should accommodate them. Since we're actually getting the file source from the filesystem (rather than relying on whether the requesting user has HTTP access) and outputting it, I think we should be very restrictive about what we allow. For example, if there was a /private/secrets.js or something, we would not want this file to be enqueued. True we're talking about CSS here, but still.

This comment has been minimized.

Copy link
@ThierryA

ThierryA Jan 23, 2018

Collaborator

Sounds good, I attempted to test on Windows as I recalled issues when doing URL to path replacements but gave up due to issues encountered when trying to run a stable local install on my virtual machine. I will leave that task to QA (Claudio).

*
* @return WP_Styles|AMP_WP_Styles Styles.
*/
protected function amp_wp_styles() {

This comment has been minimized.

Copy link
@ThierryA

ThierryA Jan 22, 2018

Collaborator

This method can be removed and AMP_Theme_Support::override_wp_styles() can be used instead if/when the comment left on the override_wp_styles() are applied.

Apply changes for code review
* Let AMP_Theme_Support::override_wp_styles() re-set global if not already instantiated.
* Move class variable definitions.
* Break up conditional into multiple lines.

kopepasah added a commit to xwp/ampnews that referenced this pull request Jan 23, 2018

@ThierryA ThierryA merged commit 4e0f5ce into develop Jan 23, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the update/css-inclusion branch Jan 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.