Opened 9 years ago
Closed 43 hours ago
#40831 closed defect (bug) (fixed)
Customize: Further improve JS inline documentation
| Reported by: | grapplerulrich | Owned by: | westonruter |
|---|---|---|---|
| Priority: | normal | Milestone: | 7.2 |
| Component: | Customize | Version: | |
| Severity: | normal | Keywords: | has-patch |
| Cc: | Focuses: | docs |
Description
We need to do another pass on the customizer code to ensure that inline documentation is added to JS. This is building on #27534, #27065, #21303, #39671 among others.
The main files that need to looked at are:
customize-base.jscustomize-loader.jscustomize-models.jscustomize-preview.jscustomize-preview-nav-menus.jscustomize-preview-widgets.jscustomize-selective-refresh.jscustomize-views.js
Related to #39930
Attachments (2)
Change History (25)
#7
@
8 years ago
- Keywords has-patch needs-refresh added; reporter-feedback needs-patch removed
- Milestone 5.1 โ Future Release
This ticket was mentioned in โPR #10743 on โWordPress/wordpress-develop by โ@westonruter.
8 months ago
#9
Trac ticket: https://core.trac.wordpress.org/ticket/40831
#11
@
6 months ago
- Milestone 7.0 โ Future Release
Now that core has TypeScript integrated into the workflow via #64662, the scope here I'm increasing to include these Customizer JS files to the list of files being type-checked.
As such, I'm punting from the current release.
โ@westonruter commented on โPR #10743:
8 days ago
#13
๐ค _Comment by Claude Opus 5_
Noting here that the comment suppressed in โthe latest review was correct and has been acted on, since suppressed comments are easy to miss.
It observed that wp.customize.Value#set() documented its to parameter as {Object} while a Value holds a value of any type. That is right, and the docblock was inconsistent with the two members either side of it in the same class, both of which already used *: initialize() for the initial value, and get() for what it returns. The QUnit tests in this pull request also call set() with a boolean and with a string. Fixed in 31e8243de0.
The review categorised it as "previously missed, in code that hasn't changed since the last review", which undersells it: the line does appear in this pull request's diff, having been reformatted when the rest parameter was added to set(). What went unexamined was the pre-existing type on a line that was already being touched.
โ@westonruter commented on โPR #10743:
8 days ago
#14
๐ค _Comment by Claude Opus 5_
Both comments suppressed in โthe latest review were correct and have been acted on in 1fa8f87576.
wp.customize.control() and wp.customize.panel() documented callback as required. Supplying several IDs without one is supported and returns a promise instead, and each docblock already said so in its own @return:
Control instance or undefined (if function called with one id param), or promise resolving to requested controls.
So the two halves of the same block disagreed. The equivalent accessors for sections and notifications already had [callback], meaning two of the four parallel blocks were right and two were wrong, which is what made it easy to miss.
Worth noting for anyone reading along: three of the last four findings on this pull request arrived suppressed, and all three were correct. Two of them, this one included, are on lines that do appear in the diff, and were categorised as "previously missed, in code that hasn't changed since the last review". The common thread is a line that was touched for one reason while a pre-existing problem elsewhere on it went unexamined.
โ@westonruter commented on โPR #10743:
8 days ago
#15
๐ค _Comment by Claude Opus 5_
All three comments suppressed in โthe latest review were correct and have been acted on in 9b6526243d.
wp.customize.Class does not always return an Object. When the prototype has an instance method the constructor returns a function instead, which is how wp.customize() and wp.customize.control() come to be callable. The QUnit tests added on this branch cover exactly that behaviour, so the documentation was contradicted by the tests sitting beside it. Now {Object|Function}, with a note on when each applies.
Partial#placements() returned {Array.<Placement>}. Placement there is a variable inside the closure rather than a documented symbol, so it resolves to nothing. Now fully qualified, consistent with the rest of the file.
This one is worth dwelling on, because it exposed a hole in the check used earlier on this branch. That check searched for the bare names as whole types, {Placement} and {Partial}, which cannot match one nested inside a generic. Re-running it against type expressions rather than whole types confirms this was the only remaining instance, but it would have been missed again.
The prose for Placement#removedNodes still described an Element. The type on that member was widened to {jQuery|DocumentFragment} earlier in this pull request, and the description immediately above it was left saying the property holds "the single Element". A member whose type and prose disagree is worse than one that is merely wrong, so thank you for catching it.
โ@westonruter commented on โPR #10743:
5 days ago
#16
๐ค _Comment by Claude Opus 5_
Following on from the review, the types touched while documenting those arguments were checked against tsconfig.json rather than assumed, which turned up three further sets. All are pushed.
The jQuery namespace is JQuery, not jQuery (14853106ba). @types/jquery declares its types under a capital J, so jQuery.Event, jQuery.jqXHR and jQuery.Promise name a namespace that does not exist:
{jQuery.Event} -> TS2749: 'jQuery.Event' refers to a value, but is being used as a type
{jQuery.jqXHR} -> TS2694: Namespace 'jQuery' has no exported member 'jqXHR'
{jQuery.Promise} -> TS2694: Namespace 'jQuery' has no exported member 'Promise'
Thirty-one occurrences are updated. jQuery on its own does resolve, so the collection type is untouched, and JQuery.Promise needs a type argument, so those read JQuery.Promise<*>. Since this branch had already adopted JQueryStatic for the global, leaving the rest of the namespace in lower case would have put both spellings in the same docblock.
Underscore and the promise casing (ffa1007be3). Nine promise types were spelled jQuery.promise against five already capitalised; Promise names a type, while promise() is the method.
Arrays now say what they contain (4f8350c807). Twenty-two gave no element type. Each was read out of the function rather than guessed from the parameter name, which mattered in at least two places: the lists compared by areElementListsEqual() are gathered with _.pluck( โฆ, 'headContainer' ) and so hold jQuery objects rather than elements, and _getInputState() returns an array only for a multiple select, whose options give strings.
Worth noting for anyone weighing this against โhttps://github.com/WordPress/wordpress-develop/pull/13251: the capital-J spelling is the correct one there too, and the same three namespace members appear a further eleven times elsewhere in src/js.
โ@afercia commented on โPR #10743:
3 days ago
#17
In other JS files we now have a few occurrences of:
{jQuery.Promise}lowercase j{jQuery.promise}lowercase j and p{jQuery.Event}lowercase j
Should we fix them in another PR?
โ@westonruter commented on โPR #10743:
3 days ago
#18
In other JS files we now have a few occurrences of:
Yes, let's keep the changes here limited to the Customizer JS files.
![(please configure the [header_logo] section in trac.ini)](/chrome/site/your_project_logo.png)
Some documentation and code improvements.