chore(@wordpress/dom): improve accuracy of jsdoc #16352
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -64,9 +64,9 @@ function isSelectionForward( selection ) { | |
| * horizontal position by default. Set `onlyVertical` to true to check only | ||
| * vertically. | ||
| * | ||
| * @param {HTMLElement} container Focusable element. | ||
| * @param {boolean} isReverse Set to true to check left, false to check right. | ||
| * @param {boolean} onlyVertical Set to true to check only vertical position. | ||
| * | ||
| * @return {boolean} True if at the edge, false if not. | ||
| */ | ||
|
|
@@ -169,8 +169,8 @@ function isEdge( container, isReverse, onlyVertical ) { | |
| /** | ||
| * Check whether the selection is horizontally at the edge of the container. | ||
| * | ||
| * @param {HTMLElement} container Focusable element. | ||
| * @param {boolean} isReverse Set to true to check left, false for right. | ||
| * | ||
| * @return {boolean} True if at the horizontal edge, false if not. | ||
| */ | ||
|
|
@@ -181,7 +181,7 @@ export function isHorizontalEdge( container, isReverse ) { | |
| /** | ||
| * Check whether the selection is vertically at the edge of the container. | ||
| * | ||
| * @param {HTMLElement} container Focusable element. | ||
| * @param {boolean} isReverse Set to true to check top, false for bottom. | ||
| * | ||
| * @return {boolean} True if at the vertical edge, false if not. | ||
|
|
@@ -255,8 +255,8 @@ export function computeCaretRect() { | |
| /** | ||
| * Places the caret at start or end of a given element. | ||
| * | ||
| * @param {HTMLElement|undefined} container Focusable element. | ||
| * @param {boolean} isReverse True for end, false for start. | ||
| */ | ||
| export function placeCaretAtHorizontalEdge( container, isReverse ) { | ||
| if ( ! container ) { | ||
|
|
@@ -364,10 +364,10 @@ function hiddenCaretRangeFromPoint( doc, x, y, container ) { | |
| /** | ||
| * Places the caret at the top or bottom of a given element. | ||
| * | ||
| * @param {HTMLElement|undefined} container Focusable element. | ||
| * @param {boolean} isReverse True for bottom, false for top. | ||
| * @param {DOMRect} [rect] The rectangle to position the caret with. | ||
| * @param {boolean} [mayUseScroll=true] True to allow scrolling, false to disallow. | ||
| */ | ||
| export function placeCaretAtVerticalEdge( container, isReverse, rect, mayUseScroll = true ) { | ||
| if ( ! container ) { | ||
|
|
@@ -484,7 +484,7 @@ export function documentHasSelection() { | |
| * Check whether the contents of the element have been entirely selected. | ||
| * Returns true if there is no possibility of selection. | ||
| * | ||
| * @param {HTMLElement} element The element to check. | ||
| * | ||
| * @return {boolean} True if entirely selected, false if not. | ||
| */ | ||
|
|
@@ -529,28 +529,28 @@ export function isEntirelySelected( element ) { | |
| } | ||
|
|
||
| /** | ||
| * Given a DOM element, finds the closest scrollable container element. | ||
| * | ||
| * @param {Element} element Element from which to start. | ||
| * | ||
| * @return {Element|undefined} Scrollable container node, if found. | ||
| */ | ||
| export function getScrollContainer( element ) { | ||
|
There was a problem hiding this comment. Changed to There was a problem hiding this comment.
Technically There was a problem hiding this comment. Correct. But |
||
| if ( ! element ) { | ||
| return; | ||
| } | ||
|
|
||
| // Scrollable if scrollable height exceeds displayed... | ||
| if ( element.scrollHeight > element.clientHeight ) { | ||
| // ...except when overflow is defined to be hidden or visible | ||
| const { overflowY } = window.getComputedStyle( element ); | ||
| if ( /(auto|scroll)/.test( overflowY ) ) { | ||
| return element; | ||
| } | ||
| } | ||
|
|
||
| // Continue traversing | ||
| return getScrollContainer( element.parentElement ); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -562,7 +562,7 @@ export function getScrollContainer( node ) { | |
| * | ||
| * @param {Node} node Node from which to find offset parent. | ||
| * | ||
| * @return {Element|null} Offset parent. | ||
| */ | ||
| export function getOffsetParent( node ) { | ||
| // Cannot retrieve computed style or offset parent only anything other than | ||
|
|
@@ -590,9 +590,8 @@ export function getOffsetParent( node ) { | |
| /** | ||
| * Given two DOM nodes, replaces the former with the latter in the DOM. | ||
| * | ||
| * @param {Node} processedNode Node to be removed. | ||
| * @param {Node} newNode Node to be inserted in its place. | ||
| */ | ||
| export function replace( processedNode, newNode ) { | ||
| insertAfter( newNode, processedNode.parentNode ); | ||
|
|
@@ -602,8 +601,7 @@ export function replace( processedNode, newNode ) { | |
| /** | ||
| * Given a DOM node, removes it from the DOM. | ||
| * | ||
| * @param {Node} node Node to be removed. | ||
| */ | ||
| export function remove( node ) { | ||
| node.parentNode.removeChild( node ); | ||
|
|
@@ -613,9 +611,8 @@ export function remove( node ) { | |
| * Given two DOM nodes, inserts the former in the DOM as the next sibling of | ||
| * the latter. | ||
| * | ||
| * @param {Node} newNode Node to be inserted. | ||
| * @param {Node} referenceNode Node after which to perform the insertion. | ||
| */ | ||
| export function insertAfter( newNode, referenceNode ) { | ||
| referenceNode.parentNode.insertBefore( newNode, referenceNode.nextSibling ); | ||
|
|
@@ -625,8 +622,6 @@ export function insertAfter( newNode, referenceNode ) { | |
| * Unwrap the given node. This means any child nodes are moved to the parent. | ||
| * | ||
| * @param {Node} node The node to unwrap. | ||
| */ | ||
| export function unwrap( node ) { | ||
| const parent = node.parentNode; | ||
|
|
@@ -638,13 +633,15 @@ export function unwrap( node ) { | |
| parent.removeChild( node ); | ||
| } | ||
|
|
||
| // eslint-disable-next-line valid-jsdoc | ||
|
There was a problem hiding this comment. I assume this will need to be updated, since we no longer use There was a problem hiding this comment. Yeah sorry about that. What's left to be done here? Forgot I had this PR open. (We can close this if it's too far behind) There was a problem hiding this comment.
I was going to ask the same 😄 It seems in pretty good shape otherwise, it would be good to get in. This one caught my eye though. There could also be some other more-recent changes to account for. Could do for a rebase. |
||
| /** | ||
| * Replaces the given node with a new node with the given tag name. | ||
| * | ||
| * @template {keyof HTMLElementTagNameMap} T | ||
|
There was a problem hiding this comment. This is the location of where the JSDoc will improve autocompletion and catch typos. There was a problem hiding this comment.
But it's not valid JSDoc ? There was a problem hiding this comment. Not valid per eslint I guess. There was a problem hiding this comment. Where is the There was a problem hiding this comment. It's an extension of jsdoc that is used by the javascript/typescript language service (LSP) that is used by most editors. You can find more on it here |
||
| * @param {Node} node The node to replace. | ||
| * @param {T} tagName The new tag name. | ||
| * | ||
| * @return {HTMLElementTagNameMap[T]} The new node. | ||
| */ | ||
| export function replaceTag( node, tagName ) { | ||
| const newNode = node.ownerDocument.createElement( tagName ); | ||
|
|
@@ -661,8 +658,8 @@ export function replaceTag( node, tagName ) { | |
| /** | ||
| * Wraps the given node with a new node with the given tag name. | ||
| * | ||
| * @param {Node} newNode The node to insert. | ||
|
There was a problem hiding this comment. Changed to |
||
| * @param {Node} referenceNode The node to wrap. | ||
| */ | ||
| export function wrap( newNode, referenceNode ) { | ||
| referenceNode.parentNode.insertBefore( newNode, referenceNode ); | ||
|
|
||
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.
Not sure the documentation generator is prepared for this.
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 looks like this is generated by this part of the JSDoc which is not to be found in the JSDoc standard.
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.
I'l defer this to @aduth but I was under the impression that we were not following the core jsdoc standard to the letter, but instead the one that is picked up by the language service (i.e. tsdoc)
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.
I think if we want to have any hope of achieving
tsctypes checking (#18838), we'll need to target the TypeScript syntax extensions. Even something such as sharing types across files (import) would not be straight-forward otherwise.We'll need some clarity and guidelines around how this is rolled out, though. I've included it in the agenda for tomorrow's #core-js meeting:
https://docs.google.com/document/d/1TJ06NiyLzThsplXdEhya51C13WE62nHN71kt997hXHw/edit#heading=h.quypt74go9w8
For example, some of the more advanced syntax like intersection types (often used for makeshift type inheritence) are both explicitly unsupported in JSDoc and flagged as invalid syntax by
eslint-plugin-jsdoc(more specifically,jsdoctypeparser, related issue). Some of this is rather open-ended on whether it should be parsed correctly [1] [2], but in the meantime we'll need to decide whether to use it, and how to work around potential interoperability issues.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.
Thanks for the context of where this is coming from. Though, if we want to merge this particular PR before the changes that need to happen to our linting/docs tooling, we need to use the JSDoc syntax at the moment and revisit later.