476: Fix javascript registration #500
Conversation
| wp_register_script( 'gp-editor', $url . '/editor.js', array( 'gp-common', 'jquery-ui-tooltip' ), '20160329' ); | ||
| wp_register_script( 'gp-glossary', $url . '/glossary.js', array( 'gp-common', 'gp-editor' ), '20160329' ); | ||
| wp_register_script( 'gp-translations-page', $url . '/translations-page.js', array( 'gp-common', 'gp-editor' ), '20150430' ); | ||
| wp_register_script( 'gp-mass-create-sets-page', $url . '/mass-create-sets-page.js', array( 'gp-common', 'gp-editor' ), '20150430' ); |
There was a problem hiding this comment.
gp-editor has already gp-common as a dependency, so we don't have to repeat it for the others.
There was a problem hiding this comment.
I tossed back and forth a few times on how best to approach that, in the end I decided it was better to be verbose in the dependencies instead of nesting them. Makes it clear what each script has for a dependency, however I'm fine with doing it the other way as well.
That's why in the template files I included all dependencies even though just including one would sometimes be all that was technically required.
There was a problem hiding this comment.
in the end I decided it was better to be verbose in the dependencies instead of nesting them.
Well, if you want to go this route you have to define 'jquery' too. :)
There was a problem hiding this comment.
True, I'm not I'll update it to nest them.
toolstack
force-pushed
the
476-fix-javascript-registration-alternate
branch
from
24a3422
to
8865703
Compare
ocean90
force-pushed
the
476-fix-javascript-registration-alternate
branch
from
8865703
to
ac5174d
Compare
| * Register the GlotPress styles | ||
| * | ||
| * @param WP_Styles $styles | ||
| * Register the GlotPress styles and load the base style sheet. |
There was a problem hiding this comment.
Docs are third-person singular elements, so third-person singular verbs should be used. (https://make.wordpress.org/core/handbook/best-practices/inline-documentation-standards/php/#language)
|
What do you think about merging |
|
I did consider merging them but I think keeping them separate is probably better, though I don't have a really good reason for that. |
|
Note that |
| * @param string|array $handles A single script handle to enqueue or an array of enqueue handles to enqueue. | ||
| */ | ||
| function gp_enqueue_script( $handles ) { | ||
| gp_enqueue_scripts( $handles ); | ||
| } |
There was a problem hiding this comment.
We shouldn't provide two functions which do the same. gp_enqueue_script() should continue to only support one handle.
function gp_enqueue_script( $handle ) {
gp_enqueue_scripts( array( $handle ) );
}Same for gp_enqueue_style().
Otherwise it will get loaded on non-GP pages for the site.
toolstack
force-pushed
the
476-fix-javascript-registration-alternate
branch
from
6fb9a93
to
3edffd5
Compare
| @@ -60,7 +60,7 @@ function gp_enqueue_styles( $handles ) { | |||
| * @param string $handle A single style handle to enqueue. | |||
| */ | |||
| function gp_enqueue_style( $handle ) { | |||
| if ( ! in_array( $handle, GP::$styles ) ) { | |||
| if ( false === in_array( $handle, GP::$styles ) ) { | |||
There was a problem hiding this comment.
What's the reason for this change? Did you mean to change it to! in_array( $handle, GP::$styles, true )?
There was a problem hiding this comment.
The coding standards check requires the comparison to be strict, hence the false === instead of !.
There was a problem hiding this comment.
OK, that was for the third argument of in_array(). in_array() returns only boolean values so there is no need for the strict check.
There was a problem hiding this comment.
Yep, just noticed that, I've updated the PR but left in the false ===.
Also revert strict check on in_array().
For better readability and consistency.
Alternate solution to #476 which uses standard WP style and script registration and enqueuing functions.