I think this documentation has errors:

    * defer If set to TRUE, the defer attribute is set on the <script> tag. Defaults to FALSE.
    * cache If set to FALSE, the JavaScript file is loaded anew on every page call, that means, it is not cached. Used only when 'type' references a JavaScript file. Defaults to TRUE.
    * preprocess Aggregate the JavaScript if the JavaScript optimization setting has been toggled in admin/config/development/performance. Note that JavaScript of type 'external' is not aggregated. Defaults to TRUE.

IMO that should be after weight into $options in "Parameters" section. I have not tested yet but other way it makes no sense for me where to pass those parameters to the function.

see drupal_get_js()

well, that would fit anywhere, maybe inside a See also section.

CommentFileSizeAuthor
#14 621902smallfix.patch8.7 KBjhodgdon
#12 drupal_add_js_docs_621902_12.patch8.57 KBmfer
#8 621902new.patch8.72 KBjhodgdon
#3 621902.patch8.78 KBjhodgdon
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

manfer’s picture

manfer CreditAttribution: manfer commented

the lines out of place that should be after weight are a little more ones:

Available constants are:

    * JS_LIBRARY: Any libraries, settings, or jQuery plugins.
    * JS_DEFAULT: Any module-layer JavaScript.
    * JS_THEME: Any theme-layer JavaScript.

If you need to invoke a JavaScript file before any other module's JavaScript, for example, you would use JS_DEFAULT - 1. Note that inline JavaScripts are simply appended to the end of the specified scope (region), so they always come last.

    * defer If set to TRUE, the defer attribute is set on the <script> tag. Defaults to FALSE.
    * cache If set to FALSE, the JavaScript file is loaded anew on every page call, that means, it is not cached. Used only when 'type' references a JavaScript file. Defaults to TRUE.
    * preprocess Aggregate the JavaScript if the JavaScript optimization setting has been toggled in admin/config/development/performance. Note that JavaScript of type 'external' is not aggregated. Defaults to TRUE.
jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Title: Documentation problem with drupal_add_js » Doc header for drupal_add_js has formatting problems

This is caused by blank lines within a @param section of a documentation header. Needs to be fixed in both Drupal 7 and Drupal 6.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Active » Needs review
FileSize
621902.patch8.78 KB

Here's a patch that should fix up the doc for drupal_add_js() -- above issues, bring it into coding standards compliance, fix some wording, grammar, spelling, etc.

jhodgdon requested that failed test be re-tested.

grendzy’s picture

grendzy CreditAttribution: grendzy commented
Status: Needs review » Needs work

patch no longer applies to HEAD.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs work » Needs review

#3: 621902.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 621902.patch, failed testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs work » Needs review
FileSize
621902new.patch8.72 KB

Here's a new patch.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

I just marked #651706: drupal_add_js() documentation needs more detail as a duplicate of this issue, because this patch (I hope) fixes all the things listed there as well.

grendzy’s picture

grendzy CreditAttribution: grendzy commented
Status: Needs review » Needs work 
+++ includes/common.inc	16 Mar 2010 22:33:46 -0000
@@ -3415,58 +3409,49 @@
+ *   - 'setting': An associative array with configuration options. The array is
+ *     directly placed in Drupal.settings. All modules should wrap their actual
+ *      configuration settings in another variable to prevent conflicts in the
+ *      Drupal.settings namespace.

it looks like there's one extra space at the beginning of the line?

+++ includes/common.inc	16 Mar 2010 22:33:46 -0000
@@ -3415,58 +3409,49 @@
+ *     on the page is very important. jQuery, for example, must be added to to

"to to" should be corrected.

Besides these 2 nits, it looks good.

Powered by Dreditor.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Good catches! Does need a small patch revision.

mfer’s picture

mfer CreditAttribution: mfer commented
Status: Needs work » Needs review
FileSize
drupal_add_js_docs_621902_12.patch8.57 KB

This patch fixes the issues from #10 and updates the copy for the weight option. It was not correct. The weight for jquery was updated, info was added about jquery.once.js, and the weighting note for inline js was removed because it is no longer relevant.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Needs work

One thing (my fault from earlier patches): The first line of the doc should say "Adds" rather than "Add" to conform with our doc standards.

Also, (again from earlier patches) maybe the code stuff here should be in a @code and all on one line:

+ *   $() being the jQuery function.  Wrap your code in (function ($) {
+ *   ... })(jQuery); or use jQuery() instead of $().

Replace with:

*   $() being the jQuery function.  Wrap your code in 
* @code
* (function ($) { ... })(jQuery);
* @endcode
* or use jQuery() instead of $().

Otherwise, looks good...

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs work » Reviewed & tested by the community
FileSize
621902smallfix.patch8.7 KB

Here's a patch with those two small changes. Taking the liberty of marking RTBC at this point...

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks!

mfer’s picture

mfer CreditAttribution: mfer commented
Status: Fixed » Reviewed & tested by the community

Looking in CVS and at the commit messages it doesn't look like #14 was committed.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Agreed, looking at drupal_add_js() in HEAD, that change hasn't been made. Good catch, mfer!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Really committed to HEAD this time. ;)

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.