-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
RichText: Add missing param docs for getActiveFormats() #23160
RichText: Add missing param docs for getActiveFormats() #23160
Conversation
…-text-src-get-active-formats-missing-params
* @param {Array} EMPTY_ACTIVE_FORMATS Array to return if there are no active | ||
* formats. | ||
* @param {Object} value Value to inspect. | ||
* @param {Array<Object>} value.formats Formats object data values. |
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.
There are no objects at the positions, but rather arrays. Each position can contain multiple formats. The position can also be empty, but I'm not sure how this would be documented.
For example, formats
can be: [ , , [ { type: 'core/bold' } ] ]
.
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.
* @param {Array<Object>} value.formats Formats object data values. | |
* @param {Array<Array>} value.formats Formats object data values. |
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 @ellatrix , I have made changes as per your suggestion.
* @param {Object} value Value to inspect. | ||
* @param {Array} EMPTY_ACTIVE_FORMATS Array to return if there are no active | ||
* formats. | ||
* @param {Object} value Value to inspect. |
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.
Same as your other PR, it would be good to create a type for a rich text value, then use it an all the other documentation. Might be good to also combine all rich text documentation updates into one PR.
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.
See https://jsdoc.app/tags-typedef.html for type definitions. This would definitely be useful as this rich text value types is used numerous times.
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.
This PR fixes existing warnings, can we merge it as is? The issue you mentioned existed before.
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.
Yes, This PR fixed the existing warning issue. But @ellatrix tells something about typedef
.
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 fixing those warnings 👍
Congratulations on your first merged pull request, @kishanjasani! We'd like to credit you for your contribution in the post announcing the next WordPress release, but we can't find a WordPress.org profile associated with your GitHub account. When you have a moment, visit the following URL and click "link your GitHub account" under "GitHub Username" to link your accounts: https://profiles.wordpress.org/me/profile/edit/ And if you don't have a WordPress.org account, you can create one on this page: https://login.wordpress.org/register Kudos! |
See #22907.
Fixes the JSDoc warnings of file:
packages/rich-text/src/get-active-formats.js
How has this been tested?
npm run lint-js packages/rich-text/src
Types of changes
Bug Fix