-
Notifications
You must be signed in to change notification settings - Fork 5
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
Enqueued script warning for wp-editor on widgets screen #531
Conversation
@shadyvb I have created this as a draft just to share my initial implementation. I may be overthinking this, but I feel there could be more to it rather than what I have implemented. There may be a possibility that we'll need to factor in the new Initially I was thought to potentially have a function to return the script dependancy as a solution but I was hesitant to go that route, however long term it may be a better option? The fix used in this module will be used to carry out across the rest of the modules. |
inc/branding/namespace.php
Outdated
|
||
global $pagenow; | ||
|
||
$dependants = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's dependencies rather than dependents, the other way around.
$dependants = [ | |
$dependencies = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variable name update.
I'd do the following to understand expected behavior:
So I looked up where the notice is coming from to understand what is expected, this is where the check happens, which says Then I checked how others are handling it, Jetpack comes up: code: https://github.com/Automattic/jetpack/blob/master/projects/plugins/jetpack/modules/wpcom-block-editor/class-jetpack-wpcom-block-editor.php#L311 , ref: Automattic/jetpack#20357 Re future-proof solution, we'll cross that bridge when we come to it. It's fine to target the current issue for now. |
Thanks for the feedback and direction. I do like that way of handling it! I feel this method can be used dependant on the module it's being implemented for and the effects it may have, so I will determine it per module basis. |
Hmmmm, the last change is a change in direction, you were changing the dependency earlier, but now you're not loading the asset at all. Is this expected ? Do we not want to load that specific resource on customizer/widget-editor screens ? |
Correct, so I figured that when the script is being called but un-needed in the customizer/ widgets screen then it could return early. In cases where the asset/s are needed then changing the dependency would be preferred. |
@robindevitt I can approve this if you're finished with it and can confirm it is functionally tested. |
Thanks @shadyvb I'm looking for what issues come up if I return the function early, so far Im not facing any issues. Which concerns me that I am maybe missing something else. I did identify this issue today with Reusable Blocks so there might be a need for the dependency? The automated tests all return successfully when running them. |
It doesn't seem related from looking at the actual js file we're using. you can confirm so by rolling back your change and checking the bug again. |
Rolling back to the previous changes Im getting the same test results. I feel there is more value at the moment in returning early rather than changing the dependencies. |
This PR fixes the original issue reported in #482 where the below error shown when on the widgets page:
Steps to confirm:
wp_enqueue_script() was called incorrectly.