Skip to content

Conversation

@sunyatasattva
Copy link
Contributor

@sunyatasattva sunyatasattva commented Sep 5, 2025

What?

The term-description block, as opposed to the post-title or the query-title, always showed a placeholder, even when the data was technically available (e.g. if the user was modifying a specific category template).

This PR fixes that by allowing the term-description block to either receive context from a parent (like the post-title), or fallback to the parsing of the template slug (like the query-title).

Why?

Not showing the actual content is not only a degraded user experience (as the user can't preview the template correctly without displaying the actual description), but also inconsistent with the behavior of other blocks.

How?

The PR basically implements already available patterns from post-title and query-title. This allows the block to be quite versatile and reusable in different contexts.

Testing Instructions

Preserving original functionality

  1. Open the All Archives template.
  2. Insert the term-description block.
  3. Verify that it displays a placeholder.

New functionality

  1. Create a new template for a specific Category (with a description).
  2. Insert the term-description block.
  3. Verify that the actual description is shown, and not a placeholder.

- Added `usesContext` to `block.json` for `termId` and `taxonomy`.
- Implemented `useTermDescription` hook to fetch term descriptions based on
context or fallback to template parsing.
- Updated `TermDescriptionEdit` component to display term descriptions or a
placeholder based on the fetched data.

This improves the block's functionality by allowing it to dynamically retrieve and
display term descriptions in various contexts.
@github-actions
Copy link

github-actions bot commented Sep 5, 2025

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: sunyatasattva <[email protected]>
Co-authored-by: mikachan <[email protected]>
Co-authored-by: tyxla <[email protected]>

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

@t-hamano t-hamano added [Type] Enhancement A suggestion for improvement. [Block] Term Description Affects the Term Description Block - used for displaying the description of a tag, catergory or othe labels Sep 11, 2025
@mikachan mikachan mentioned this pull request Sep 12, 2025
15 tasks
Copy link
Member

@mikachan mikachan left a comment

Choose a reason for hiding this comment

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

This is working great in my testing, nice work! I'm able to see the term description content in both the editor and the front end:

Editor Front end
image image

And I can see your approach very closely matches the query-title and post-title implementations.

I can only see the tiniest of improvements: we usually end all comments with a full stop.

</div>
{ termDescription ? (
<div
dangerouslySetInnerHTML={ { __html: termDescription } }
Copy link
Member

Choose a reason for hiding this comment

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

Should this termDescription go through some sanitization before being passed to dangerouslySetInnerHTML?

Copy link
Member

Choose a reason for hiding this comment

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

The term description here uses term_description(), which already applies sanitisation. I don't believe the content can be edited at any other point, so I'm not sure we need additional sanitisation here. This also follows the same pattern as post-content, post-title, and post-author.

* This maintains the same logic as the original implementation for cases where
* no termId/taxonomy context is provided.
*/
function useTemplateBasedTermData() {
Copy link
Member

Choose a reason for hiding this comment

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

So much duplication with useArchiveLabel() here. Could make sense to consider avoiding it and maybe adding some documentation, because it can be particularly difficult to follow some bits, like the regex match processing, for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, to be honest, as this was a PR to tentatively dip my feet into something quite spontaneous, that hadn't been previously discussed, I opted for the minimal approach which wouldn't modify anything else.

I suggest that we merge this PR in this state, and then deal with refactoring in a follow-up PR, so we keep things contained.

What do you think? My concern to keep things “atomically testable”: so we make sure this PR does whatever it says it does, and then we move on to touch other parts of the system which will require a different set of tests, and for which we have safe fallback.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me - I'm good with opening an issue so we can follow up on the deduplication at a later point.

Comment on lines 106 to 108
return {
termDescription,
};
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, I don't see a good reason to return an object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to keep the return type consistent with useArchiveLabel, so that, when used, these kinds of hooks require the least amount of thought from the consumers as possible, and they look more readable together. Pushing a commit to change this, however, we can always revert it to this type if you think my reasoning makes sense.

Copy link
Member

Choose a reason for hiding this comment

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

I think I prefer the flat version 😉

IMO, the only time when an object with a single property is fine is when we expect that the return value will grow in time and we don't yet have the design for it, but we want to minimize breaking changes and integration cost.

}, [] );

const taxonomyMatches = templateSlug?.match(
/^(category|tag|taxonomy-([^-]+))$|^(((category|tag)|taxonomy-([^-]+))-(.+))$/
Copy link
Member

Choose a reason for hiding this comment

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

Would love to see this documented. At first glance, the category|tag|taxonomy- vs (category|tag)|taxonomy- looks like a mistake.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the same RegEx used in useArchiveLabel, copy+pasted. See my comment above about refactoring: I think in a followup PR we can consolidate and document.

(Besides, I see some dirty code in useArchiveLabel anyways that could use a cleanup PR).

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, happy to have that in a follow up 👍 just let's make sure it doesn't fall through the cracks (document it in an issue or in TODO as a comment)

Comment on lines +45 to +48
// Access core/editor by string to avoid @wordpress/editor dependency.
// eslint-disable-next-line @wordpress/data-no-store-string-literals
const { getCurrentPostId, getCurrentPostType, getCurrentTemplateId } =
select( 'core/editor' );
Copy link
Member

Choose a reason for hiding this comment

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

I do think this is a bit messed up. We don't really avoid a @wordpress/editor dependency, do we? So why pretend? Looks like something we should address further down the line.

Copy link
Member

Choose a reason for hiding this comment

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

I think we're mainly avoiding @wordpress/block-library from being dependent on @wordpress/editor, as blocks can be loaded into a non-post block editor. We're doing something similar for useArchiveLabel(), so if we're going to address this in a different way, then perhaps we could handle both of these cases at once in a separate PR (and probably a few other similar cases).

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense and sounds good to be addressed together and separately from this PR.

const hasContext = Boolean( termId && taxonomy );

return {
hasContext,
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to expose hasContext?

Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing this might be for extensibility reasons, but I'm sure @sunyatasattva can confirm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, exactly. This was added to make sure that, downstream, the block can know whether the result of the content is coming from the template data or from the passed down context. This is useful when this block is going to be used as an inner block of some arbitrary parent (in one example case: Featured Category block in WooCommerce).

hasContext,
setDescription,
termDescription: hasContext
? fullDescription?.rendered || description || ''
Copy link
Member

Choose a reason for hiding this comment

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

Should the empty string fallback be at the end of all the logic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mm, not sure I get what you mean. You mean after the whole ternary? Because the other side of the ternary (templateBasedData) already falls back to an empty string.

Copy link
Member

Choose a reason for hiding this comment

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

I'm looking at the code and can't recall why I suggested this. Maybe it was because of the '' fallbacks in other places in the code. Feel free to disregard 👍

return useSelect(
( select ) => {
if ( ! taxonomy || ! termSlug ) {
return { termDescription: '' };
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to return an object? Seems like we can just return the string there.

@sunyatasattva
Copy link
Contributor Author

@tyxla @mikachan I pushed the requested changes, let me know if I have to do anything else. Someone else will need to merge as I don't have permission.

@tyxla
Copy link
Member

tyxla commented Sep 22, 2025

I'll let @mikachan do another round of review, as I don't have much review cycles this week - thanks! 🐢

Copy link
Member

@mikachan mikachan left a comment

Choose a reason for hiding this comment

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

All the changes look good to me, thanks @sunyatasattva!

Looks like we have a few follow-up tasks:

  • Reduce the duplication with useArchiveLabel and useTemplateBasedTermData: #71525 (comment)
  • Consolidate and document the regex used in both hooks: #71525 (comment)
  • Address the workarounds for avoiding being dependent on @wordpress/editor in @wordpress/block-library: #71525 (comment)

I think this is ready to bring in 🚢 ✨

@mikachan mikachan merged commit e02df75 into WordPress:trunk Sep 22, 2025
69 of 70 checks passed
@github-actions github-actions bot added this to the Gutenberg 21.8 milestone Sep 22, 2025
@ntsekouras
Copy link
Contributor

Shouldn't this PR also handle the php rendering of the block when the context is provided?

@mikachan
Copy link
Member

Shouldn't this PR also handle the php rendering of the block when the context is provided?

I have a fix here: #72238

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Block] Term Description Affects the Term Description Block - used for displaying the description of a tag, catergory or othe [Type] Enhancement A suggestion for improvement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants