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

Optimize scripts/styles for non LifterLMS pages #146

Open
scottwyden opened this issue Jul 12, 2016 · 10 comments
Open

Optimize scripts/styles for non LifterLMS pages #146

scottwyden opened this issue Jul 12, 2016 · 10 comments
Labels
Type: Enhancement Improvements existing features or code
Milestone

Comments

@scottwyden
Copy link

scottwyden commented Jul 12, 2016

I have created this code which has stopped the Lifter scripts and styles from loading on non Lifter pages.

Feel free to tweak and use in the next release, but if you do, please alert me so I can remove my custom script.

`/* Optimize LifterLMS Scripts */
add_action( 'wp_enqueue_scripts', 'remove_lifter_scripts', 99 );

function remove_lifter_scripts() {
//first check that LifterLMS exists to prevent fatal errors
if ( function_exists( 'is_lifterlms') ) {
//dequeue scripts and styles
if ( !is_lifterlms() && !is_llms_shop() && !is_course_taxonomy() && !is_course() && !is_lesson() ) {
wp_dequeue_script( 'llms' );
wp_dequeue_script( 'llms-ajax' );
wp_dequeue_script( 'llms-form-checkout' );
wp_dequeue_script( 'llms-lesson-locked' );
wp_dequeue_script( 'jquery-ui-datepicker' );
wp_dequeue_script( 'jquery-ui-slider' );
wp_dequeue_script( 'chosen-jquery' );
wp_dequeue_script( 'collapse' );
wp_dequeue_script( 'transition' );
wp_dequeue_style( 'temp-styles' );
wp_dequeue_style( 'certificates' );
wp_dequeue_style( 'chosen-styles' );
wp_dequeue_style( 'admin-styles' );
}
}

}`

@thomasplevy
Copy link
Contributor

@scottwyden this is helpful but isn't usable code I can integrate into the codebase as it's not a real fix, just a patch.

The ideal solution we need here is to rewrite the actual enqueue scripts or methods so that things are only enqueued when they need to be...

Thanks for your effort here and if you're willing, please look at https://github.com/gocodebox/lifterlms/blob/master/includes/class.llms.frontend.assets.php where the assets are originally enqueued.

I'll leave the issue open and get it resolved as per my note in the topic at wordpress.org but I can't use this script in the codebase, sorry!

@thomasplevy thomasplevy added Type: Enhancement Improvements existing features or code help wanted Looking for contributors to assist with this issue labels Jul 12, 2016
@scottwyden
Copy link
Author

scottwyden commented Jul 12, 2016

I'm not a coder otherwise I'd totally help.

@thomasplevy
Copy link
Contributor

@scottwyden I completely understand. We'll leave this open until we get a chance to rework it internally then.

Take care,

@scottwyden
Copy link
Author

Sounds great :-)

@BurlesonBrad
Copy link

I'm not going to rewrite your code for you,

...misses point of community / open source :where's-the-homer-doh-emoji-when-I-need-it:
but I'll make an attempt and try to keep my Texas Sized thoughts to myself

a simple fork and a merge wouldn't have hurt @scottwyden 😎 Hell, 'ya already did the (great) work 👍

Ok, pardon me b/c I'm in video editing mode right now and the only thing rattlin' through my puny medulla oblongata is my wife pointing out how many ums I used..... but: I'd rather contribute anything always, than something great never! ~ha!

Shouldn't (something like) this

if ( function_exists( 'wpcf7_enqueue_scripts' ) ) {
        wpcf7_enqueue_scripts();
    }

(replacing CF7 w/ lifterlms of course)
be placed somewhere around line 55 of class.llms.frontend.assets.php so that it wraps up this: https://github.com/gocodebox/lifterlms/blob/master/includes/class.llms.frontend.assets.php#L55-L74

Likewise, something like this here:

if ( function_exists( 'wpcf7_enqueue_styles' ) ) {
        wpcf7_enqueue_styles();
    }

would be somewhere yonder near line 33 of class.llms.frontend.assets.php so that it takes care of this:
https://github.com/gocodebox/lifterlms/blob/master/includes/class.llms.frontend.assets.php#L55-L74

#NotAfraidToBeWrong #AlwaysBeLearning

@thomasplevy thomasplevy removed good first issue If you're a first time contributor this is a good issue for you! help wanted Looking for contributors to assist with this issue labels Mar 27, 2018
@actual-saurabh
Copy link
Contributor

This seems to be the next logical step in script loading and optimisation after #484.

@actual-saurabh
Copy link
Contributor

Will refactor some of the enqueuing mechanisms alongside #562.

@actual-saurabh
Copy link
Contributor

Thinking aloud. Do let me know if I am missing something here (esp @thomasplevy):

There are only three situations when LifterLMS related scripts need to be loaded:

  1. When we are on specific LifterLMS course element.
  2. When we are on a page that uses a specific LifterLMS shortcode.
  3. When we are on a page that uses a LifterLMS related sidebar widget.

I'll dig into more specifics of exactly what script is needed for each such scenario and specific sub-scenarios.

For shortcodes, I considered using has_shortcode but that has a performance issue. However, if the asset (script or style) is registered separately but only enqueued inside the shortcode's output function, it will only get enqueued on the page where the shortcode is actually used. I looked for some confirmation and found one here: https://gist.github.com/mathetos/c665f24cd7d8d8a1fbad63a8d5c9a741#file-better_shortcode-php & https://www.thewpcrowd.com/wordpress/enqueuing-scripts-only-when-widget-or-shortcode/

In a similar way, widget related scripts can be loaded only on pages that contain the widget. See: https://wordpress.stackexchange.com/a/48346/20375

However, this means that we'd need to refactor asset enqueuing from the current mechanism. In https://github.com/gocodebox/lifterlms/blob/master/includes/class.llms.frontend.assets.php, we only register all scripts and move the actual enqueuing inside each individual module. Or better still, move all asset management within the module that needs such an asset.

This also works towards the goal of having self-contained modules that can just hooked and unhooked from the main plugin in a plugins-within-a-plugin kind of architecture.

With the way wp_enqueue_script() and related work, duplication isn't an issue since in the end only one copy is going to be enqueued on one application state.

@actual-saurabh
Copy link
Contributor

Second, as far as printing scripts inline is concerned, we mainly need that to create a global JS object. See: https://github.com/gocodebox/lifterlms/blob/master/includes/class.llms.frontend.assets.php#L189-L201 and https://github.com/gocodebox/lifterlms/blob/master/includes/class.llms.frontend.assets.php#L96-L100

So, we could do exactly what WordPress core does, i.e., print this global object in the header or create a new js file and load this as the first file in the footer.

In tandem, every other script we register would always have this as a dependency, thereby ensuring that the object is always available when any of the scripts need it.

@Micu22
Copy link

Micu22 commented Apr 17, 2021

While going through the documentation I stumbled upon this:
https://developer.lifterlms.com/reference/classes/llms_frontend_assets/enqueue_scripts/
The first lines say:

    // I don't think we need these next 3 scripts.
    wp_enqueue_script( 'jquery-ui-tooltip' );
    wp_enqueue_script( 'jquery-ui-datepicker' );
    wp_enqueue_script( 'jquery-ui-slider' );
 
    llms()->assets->enqueue_script( 'webui-popover' );

My guess is the first optimization thing you could do is remove jquery-ui, and if something from it is needed, then write it in your own js file, reduces requests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement Improvements existing features or code
Projects
None yet
Development

No branches or pull requests

5 participants