Skip to content
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

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

fabiankaegy
Copy link
Member

What?

Adds a new Term type to the available entity types exported from the core-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.

@fabiankaegy fabiankaegy added [Type] Code Quality Issues or PRs that relate to code quality [Package] Core data /packages/core-data labels Jun 12, 2024
@fabiankaegy fabiankaegy self-assigned this Jun 12, 2024
@fabiankaegy fabiankaegy requested a review from nerrad as a code owner June 12, 2024 07:20
Copy link

github-actions bot commented Jun 12, 2024

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: fabiankaegy <fabiankaegy@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>
Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@Mamaduka
Copy link
Member

Unfortunately, I'm not the best person to review TS changes.

cc @tyxla, @jsnajdr

Copy link
Member

@tyxla tyxla left a 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 };
Copy link
Member

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?

Suggested change
meta: { [ key: string ]: unknown };
meta: ContextualField<
Record< string, string >,
'view' | 'edit',
C
>;
/**
* The title for the term.
*/
name: string;
Copy link
Member

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:

Suggested change
name: string;
name: ContextualField< string, 'view' | 'embed' | 'edit', C >;
/**
* An alphanumeric identifier for the term.
*/
slug: string;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above:

Suggested change
slug: string;
slug: ContextualField< string, 'view' | 'embed' | 'edit', C >;
*/
name: string;
/**
* An alphanumeric identifier for the term.
Copy link
Member

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;
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Core data /packages/core-data [Type] Code Quality Issues or PRs that relate to code quality
3 participants