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

Adds documentation for WordPress.WP.EnqueuedResources. #1824

Conversation

NielsdeBlaauw
Copy link
Contributor

@NielsdeBlaauw NielsdeBlaauw commented Oct 31, 2019

Related to #1722

@jrfnl jrfnl added this to In progress in XML Documentation via automation Nov 1, 2019
@NielsdeBlaauw NielsdeBlaauw force-pushed the 1722-WordPress.WP.EnqueuedResources branch from 0efe230 to b6e50ae Compare November 2, 2019 21:19
Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @NielsdeBlaauw Looking good.

I've left some small remarks inline.

Aside from that, I'm wondering if it would be better for the example code to reference local files instead of external URLs, as that is (or should be) the more common use-case.

For plugins, this would be something like:
plugins_url( 'css/my-css.css', __FILE__ ),

And for themes:
get_template_directory_uri() . '/js/nav.js'

WordPress/Docs/WP/EnqueuedResourcesStandard.xml Outdated Show resolved Hide resolved
WordPress/Docs/WP/EnqueuedResourcesStandard.xml Outdated Show resolved Hide resolved
WordPress/Docs/WP/EnqueuedResourcesStandard.xml Outdated Show resolved Hide resolved
@NielsdeBlaauw
Copy link
Contributor Author

Aside from that, I'm wondering if it would be better for the example code to reference local files instead of external URLs, as that is (or should be) the more common use-case.

I'm worried the code for the invalid example might be too far from what the user is actually doing, or what the sniff will be picking up. It's not about what resource is loaded, it is more about the way it is being done. IMHO this example is easier and more accessible to people with limited knowledge of WP. Also it avoids a discussion of where the actual resource is coming from.

@NielsdeBlaauw NielsdeBlaauw removed their assignment Nov 6, 2019
@jrfnl
Copy link
Member

jrfnl commented Nov 6, 2019

Aside from that, I'm wondering if it would be better for the example code to reference local files instead of external URLs, as that is (or should be) the more common use-case.

I'm worried the code for the invalid example might be too far from what the user is actually doing, or what the sniff will be picking up. It's not about what resource is loaded, it is more about the way it is being done. IMHO this example is easier and more accessible to people with limited knowledge of WP. Also it avoids a discussion of where the actual resource is coming from.

True enough. Maybe using a $path_to_file variable would simplify it in that case ?

The reason I'm not keen to leave the examples with the URLs is two-fold and revolves around other problems which this sniff is not sniffing for:

  1. People using a CDN-version of script which WP natively ships with, often at a different version than the one which ships with WP.
    This breaks other plugins/themes as those expect the WP native version and their code may break when a different version is used. Also see Extra: Sniff to check for overruling of WP included scripts #867
  2. People using just a CDN-version of a script without fall-back to local.
    This can break sites pretty badly when:
    • the CDN is down;
    • the CDN stops hosting a particular script or the particular version being pointed to;
    • the pointer is to a version-less script name and the version of the script on the CDN changes.

These are issues outside the scope of the sniff, so I agree that we shouldn't address them here, but I do believe showing code which we may well forbid at a later stage, is not a good idea either.

@NielsdeBlaauw
Copy link
Contributor Author

NielsdeBlaauw commented Nov 6, 2019

Agree, I'm going to replace it with a variable.

@NielsdeBlaauw NielsdeBlaauw force-pushed the 1722-WordPress.WP.EnqueuedResources branch from 542ce59 to 9bd5f9c Compare November 6, 2019 21:06
Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

XML Documentation automation moved this from In progress to Reviewer approved Nov 6, 2019
@GaryJones GaryJones merged commit e105835 into WordPress:develop Nov 7, 2019
XML Documentation automation moved this from Reviewer approved to Done Nov 7, 2019
@jrfnl jrfnl added this to the 2.2.0 milestone Nov 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants