-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Add entity type for individual Term objects #62504
base: trunk
Are you sure you want to change the base?
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
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 looks pretty good, thanks @fabiankaegy 🙌
The primary thing that we should check is whether we're correctly providing the context of each field. Or perhaps you're intentionally skipping them?
Other than that, I think this is good to go!
/** | ||
* Meta fields for the term. | ||
*/ | ||
meta: { [ key: string ]: unknown }; |
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.
Isn't meta essentially an associative array of meta fields? And because of the database schema, they'll always end up strings?
Also it supports view
and edit
contexts, right?
meta: { [ key: string ]: unknown }; | |
meta: ContextualField< | |
Record< string, string >, | |
'view' | 'edit', | |
C | |
>; |
/** | ||
* The title for the term. | ||
*/ | ||
name: string; |
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.
According to the REST API docs, this can be available in multiple contexts:
name: string; | |
name: ContextualField< string, 'view' | 'embed' | 'edit', C >; |
/** | ||
* An alphanumeric identifier for the term. | ||
*/ | ||
slug: string; |
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 above:
slug: string; | |
slug: ContextualField< string, 'view' | 'embed' | 'edit', C >; |
*/ | ||
name: string; | ||
/** | ||
* An alphanumeric identifier for the term. |
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've always found the "alphanumeric" description ambiguous, considering that terms can also contain dashes.
/** | ||
* The number of objects with the term. | ||
*/ | ||
count: number; |
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'd suggest auditing all fields for context. count
seems to be available for view
and edit
and not for embed
.
What?
Adds a new
Term
type to the available entity types exported from thecore-data
package.Why?
' getEntityRecord' is commonly used to deal with individual terms outside of any taxonomy. Currently, we don't expose any types for it.
How?
Adding the type to the correct folder and exporting it.