-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Enhance Term Description Block with Context Support #71525
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,18 +13,27 @@ import { | |
| AlignmentControl, | ||
| } from '@wordpress/block-editor'; | ||
|
|
||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import { useTermDescription } from './use-term-description'; | ||
|
|
||
| export default function TermDescriptionEdit( { | ||
| attributes, | ||
| setAttributes, | ||
| mergedStyle, | ||
| context: { termId, taxonomy }, | ||
| } ) { | ||
| const { textAlign } = attributes; | ||
| const { termDescription } = useTermDescription( termId, taxonomy ); | ||
|
|
||
| const blockProps = useBlockProps( { | ||
| className: clsx( { | ||
| [ `has-text-align-${ textAlign }` ]: textAlign, | ||
| } ), | ||
| style: mergedStyle, | ||
| } ); | ||
|
|
||
| return ( | ||
| <> | ||
| <BlockControls group="block"> | ||
|
|
@@ -36,9 +45,15 @@ export default function TermDescriptionEdit( { | |
| /> | ||
| </BlockControls> | ||
| <div { ...blockProps }> | ||
| <div className="wp-block-term-description__placeholder"> | ||
| <span>{ __( 'Term Description' ) }</span> | ||
| </div> | ||
| { termDescription ? ( | ||
| <div | ||
| dangerouslySetInnerHTML={ { __html: termDescription } } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The term description here uses |
||
| /> | ||
| ) : ( | ||
| <div className="wp-block-term-description__placeholder"> | ||
| <span>{ __( 'Term Description' ) }</span> | ||
| </div> | ||
| ) } | ||
| </div> | ||
| </> | ||
| ); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,112 @@ | ||
| /** | ||
| * WordPress dependencies | ||
| */ | ||
| import { store as coreStore, useEntityProp } from '@wordpress/core-data'; | ||
| import { useSelect } from '@wordpress/data'; | ||
|
|
||
| /** | ||
| * Hook to fetch term description based on context or fallback to template parsing. | ||
| * | ||
| * This hook prioritizes context-provided termId and taxonomy, but falls back to | ||
| * template-based detection when no context is available. | ||
| * | ||
| * @param {string|number} termId The term ID from context | ||
| * @param {string} taxonomy The taxonomy name from context | ||
| */ | ||
| export function useTermDescription( termId, taxonomy ) { | ||
| const [ description, setDescription, fullDescription ] = useEntityProp( | ||
| 'taxonomy', | ||
| taxonomy, | ||
| 'description', | ||
| termId | ||
| ); | ||
|
|
||
| // Fallback approach: Parse template slug when no context is available | ||
mikachan marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| const templateBasedData = useTemplateBasedTermData(); | ||
|
|
||
| const hasContext = Boolean( termId && taxonomy ); | ||
|
|
||
| return { | ||
| hasContext, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need to expose
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). |
||
| setDescription, | ||
| termDescription: hasContext | ||
| ? fullDescription?.rendered || description || '' | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 (
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| : templateBasedData.termDescription, | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Fallback hook to fetch term data from template context (backward compatibility). | ||
| * This maintains the same logic as the original implementation for cases where | ||
| * no termId/taxonomy context is provided. | ||
| */ | ||
| function useTemplateBasedTermData() { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So much duplication with
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
| const templateSlug = useSelect( ( select ) => { | ||
| // 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' ); | ||
|
Comment on lines
+45
to
+48
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we're mainly avoiding
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 currentPostType = getCurrentPostType(); | ||
| const templateId = | ||
| getCurrentTemplateId() || | ||
| ( currentPostType === 'wp_template' ? getCurrentPostId() : null ); | ||
|
|
||
| return templateId | ||
| ? select( coreStore ).getEditedEntityRecord( | ||
| 'postType', | ||
| 'wp_template', | ||
| templateId | ||
| )?.slug | ||
| : null; | ||
| }, [] ); | ||
|
|
||
| const taxonomyMatches = templateSlug?.match( | ||
| /^(category|tag|taxonomy-([^-]+))$|^(((category|tag)|taxonomy-([^-]+))-(.+))$/ | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would love to see this documented. At first glance, the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the same RegEx used in (Besides, I see some dirty code in
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) |
||
| ); | ||
|
|
||
| let taxonomy; | ||
| let termSlug; | ||
|
|
||
| if ( taxonomyMatches ) { | ||
| // If it's for all taxonomies of a type (e.g., category, tag) | ||
mikachan marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| if ( taxonomyMatches[ 1 ] ) { | ||
| taxonomy = taxonomyMatches[ 2 ] | ||
| ? taxonomyMatches[ 2 ] | ||
| : taxonomyMatches[ 1 ]; | ||
| } | ||
| // If it's for a specific term (e.g., category-news, tag-featured) | ||
mikachan marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| else if ( taxonomyMatches[ 3 ] ) { | ||
| taxonomy = taxonomyMatches[ 6 ] | ||
| ? taxonomyMatches[ 6 ] | ||
| : taxonomyMatches[ 4 ]; | ||
| termSlug = taxonomyMatches[ 7 ]; | ||
| } | ||
|
|
||
| taxonomy = taxonomy === 'tag' ? 'post_tag' : taxonomy; | ||
| } | ||
|
|
||
| return useSelect( | ||
| ( select ) => { | ||
| if ( ! taxonomy || ! termSlug ) { | ||
| return { termDescription: '' }; | ||
|
||
| } | ||
|
|
||
| const { getEntityRecords } = select( coreStore ); | ||
|
|
||
| const termRecords = getEntityRecords( 'taxonomy', taxonomy, { | ||
| slug: termSlug, | ||
| per_page: 1, | ||
| } ); | ||
|
|
||
| let termDescription = ''; | ||
| if ( termRecords && termRecords[ 0 ] ) { | ||
| termDescription = termRecords[ 0 ].description || ''; | ||
| } | ||
|
|
||
| return { | ||
| termDescription, | ||
| }; | ||
|
||
| }, | ||
| [ taxonomy, termSlug ] | ||
| ); | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.