search

digisavvy / some-like-it-neat

A WordPress Theme Using _s, Bourbon + Neat and Theme Hook Alliance
GNU General Public License v2.0
276 stars 61 forks source link

Clean Up scripts #97

Closed bryanwillis closed 8 years ago

bryanwillis commented 8 years ago

Lots of errors arising due to the way scripts are being included.

bryanwillis commented 8 years ago

I don't use some-like-it-neat so I'm not sure if my proposals are right. I came across this theme from a friend that was using it and a few things seemed strange with how scripts were being loaded. There may be a reason for this though (like the gulp concat of scripts/styles) and I might be totally wrong here...

Normally doing this is redundant:

wp_register_script( 'hoverintent-js', get_template_directory_uri() . '/assets/js/vendor/hoverintent/jquery.hoverIntent.js', array( 'jquery' ), '1.0.0', true );
wp_enqueue_script( 'hoverintent-js', get_template_directory_uri() . '/assets/js/vendor/hoverintent/jquery.hoverIntent.js', array( 'jquery' ), '1.0.0', true );

When you can just do this:

wp_register_script( 'hoverintent-js', get_template_directory_uri() . '/assets/js/vendor/hoverintent/jquery.hoverIntent.js', array( 'jquery' ), '1.0.0', true );
wp_enqueue_script( 'hoverintent-js');

I also noticed that modernizr and selectivizr were being loaded in the footer by default. Modernizr should be loaded in the head to avoid the fouc and so load html5shiv to render html5 elements properly. I've never used selectivizr, but according to the documentation, it also should be loaded in the head (probably for similar reasons).

I also noticed selectivizr is being loaded twice. Is there a reason for this? One is being loaded in the head and the other is being enqueued in the footer without the conditional comments so it loads for all browsers which is not ideal.

The correct way to load selectivizr would be like this:

wp_register_script( 'selectivizr-js', get_template_directory_uri() . '/assets/js/vendor/selectivizr/selectivizr.js', array( 'jquery' ), '1.0.2b', false );
wp_enqueue_script( 'selectivizr' );
wp_script_add_data( 'selectivizr', 'conditional', '(gte IE 6)&(lte IE 8)' );

It also seemed like there was a lot of redundancy here that could be shortened.

Again I could be totally off on all this as I haven't had a chance to try the theme out and only quickly looked through the functions.php file. I'm sure these changes would also require changes to the gulp file which I haven't looked at.

digisavvy commented 8 years ago

@bryanwillis excellent points, thank you. =)

Would you be willing to create these changes on the Development branch instead of master?

digisavvy commented 8 years ago

@bryanwillis thanks again for the PR. Your points are "on fleek," as the kids say. Lots of redundancies there, so thanks for that. =) After reviewing the proposed set of changes I went ahead and accepted them.

bryanwillis commented 8 years ago

Ha I'm getting too old... never heard the term "on fleek" until now. Anyway, glad it worked out ok! Sorry for not submitting the changes on the development brach... 3 years on github and I still suck at git :-1: