Conversation
|
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
|
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
|
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. |
| linkPanel={ isLinkTag ? linkPanel : null } | ||
| /> | ||
| </InspectorControls> | ||
| <InspectorControls group="advanced"> |
There was a problem hiding this comment.
What was the reason to remove this control? Is it because there'll always be an href now?
It means users can no longer switch between <a> and <button> tags, which seems on the surface to be a regression to me.
| * - type: post type slug or taxonomy slug | ||
| * - id: entity ID | ||
| * | ||
| * @since 6.9.0 |
There was a problem hiding this comment.
I know you've been waiting for a while for reviews, thanks for your patience!
I don't know if this will make the next WordPress release, but the next version is now 7.0.0
| * @param WP_Block $block_instance The block instance. | ||
| * @return mixed The value computed for the source. | ||
| */ | ||
| function gutenberg_block_bindings_entity_get_value( array $source_args, $block_instance ) { |
There was a problem hiding this comment.
It would be good to get test coverage here, e.g.,
- Valid post-type and taxonomy URL resolution
- Non-public post denial
- Password-protected post denial
- Invalid/missing args
- Deleted entity handling (?)
| } | ||
|
|
||
| $kind = (string) $source_args['kind']; | ||
| $type = (string) $source_args['type']; |
There was a problem hiding this comment.
Should we check if the post type exists? E.g.,
if ( ! post_type_exists( $type ) ) {
return null;
}| clearEntityUrlBinding, | ||
| createEntityUrlBinding, |
There was a problem hiding this comment.
I think these (and unlink()) should be wrapped in a useCallback().
They are plain closures recreated every render, which will make the useMemo recompute here.
Either that or restructure the logic somehow.
| urlBinding?.source === 'core/term-data'; | ||
| const boundEntityArgs = isEntityUrlBinding ? urlBinding?.args : null; | ||
|
|
||
| const resolvedEntityUrl = useSelect( |
There was a problem hiding this comment.
Could this useSelect be combined with the one below? They both call getEntityRecord.
| preview, | ||
| suggestionsQuery: getSuggestionsQuery( boundType, boundKind ), | ||
| help, | ||
| onSelect: ( updatedLink ) => { |
There was a problem hiding this comment.
This callback seems very similar to LinkControl.onChange - could be extracted to a shared helper?
| @@ -150,11 +150,15 @@ export function useToolsPanelItem( | |||
| } | |||
|
|
|||
| if ( isMenuItemChecked && ! isValueSet && ! wasMenuItemChecked ) { | |||
| onSelect?.(); | |||
| if ( typeof onSelect === 'function' ) { | |||
There was a problem hiding this comment.
What's the motivation behind this change?
There's a subtle difference: optional chaining would throw on non-function truthy values (surfacing bugs), while typeof silently skips them.
I only raise it because it's a shared component used in many places. If there's a good reason, this change would be better suited to its own PR.
What?
Navigation linksforbuttonblockWhy?
How?
Testing Instructions
(Optional) Paste/type a custom external URL and confirm:
The entity binding is removed and the Button uses the typed URL.
Screenshots or screencast