-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Simplify and improve navigation link creation flow #73210
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 all commits
b9ece22
79686f8
3c6df97
21b4a04
a31060b
2b621e8
26ae3ab
4aa39ff
ad70be0
7c68c2a
9a0dad8
a765874
ccf5ca9
a7aea0d
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 |
|---|---|---|
|
|
@@ -15,7 +15,7 @@ import { | |
| __experimentalInputControlSuffixWrapper as InputControlSuffixWrapper, | ||
| } from '@wordpress/components'; | ||
| import { __, sprintf } from '@wordpress/i18n'; | ||
| import { useRef, useState, useEffect } from '@wordpress/element'; | ||
| import { useRef, useState, useEffect, useMemo } from '@wordpress/element'; | ||
| import { useInstanceId } from '@wordpress/compose'; | ||
| import { focus } from '@wordpress/dom'; | ||
| import { ENTER } from '@wordpress/keycodes'; | ||
|
|
@@ -187,7 +187,15 @@ function LinkControl( { | |
| const wrapperNode = useRef(); | ||
| const textInputRef = useRef(); | ||
| const searchInputRef = useRef(); | ||
| const isEndingEditWithFocusRef = useRef( false ); | ||
| // TODO: Remove entityUrlFallbackRef and previewValue in favor of value prop after taxonomy entity binding | ||
| // is stable and returns the correct URL instead of null while resolving when creating the entity. | ||
| // | ||
| // Preserve the URL from entity suggestions before binding overrides it | ||
| // This is due to entity binding not being available immediately after the suggestion is selected. | ||
| // The URL can return null, especially for taxonomy entities, while entity binding is being resolved. | ||
| // To avoid unnecessary rerenders and focus loss, we preserve the URL from the suggestion and use it | ||
| // as a fallback until the entity binding is available. | ||
| const entityUrlFallbackRef = useRef(); | ||
|
|
||
| const settingsKeys = settings.map( ( { id } ) => id ); | ||
|
|
||
|
|
@@ -244,8 +252,6 @@ function LinkControl( { | |
| wrapperNode.current; | ||
|
|
||
| nextFocusTarget.focus(); | ||
|
|
||
| isEndingEditWithFocusRef.current = false; | ||
| }, [ isEditingLink, isCreatingPage ] ); | ||
|
|
||
| // The component mounting reference is maintained separately | ||
|
|
@@ -261,18 +267,18 @@ function LinkControl( { | |
| const hasLinkValue = value?.url?.trim()?.length > 0; | ||
|
|
||
| /** | ||
| * Cancels editing state and marks that focus may need to be restored after | ||
| * the next render, if focus was within the wrapper when editing finished. | ||
| * Cancels editing state. | ||
| */ | ||
| const stopEditing = () => { | ||
| isEndingEditWithFocusRef.current = !! wrapperNode.current?.contains( | ||
|
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.
|
||
| wrapperNode.current.ownerDocument.activeElement | ||
| ); | ||
|
|
||
| setIsEditingLink( false ); | ||
| }; | ||
|
|
||
| const handleSelectSuggestion = ( updatedValue ) => { | ||
| // Preserve the URL for taxonomy entities before binding overrides it | ||
| if ( updatedValue?.kind === 'taxonomy' && updatedValue?.url ) { | ||
| entityUrlFallbackRef.current = updatedValue.url; | ||
| } | ||
|
|
||
| // Suggestions may contains "settings" values (e.g. `opensInNewTab`) | ||
| // which should not override any existing settings values set by the | ||
| // user. This filters out any settings values from the suggestion. | ||
|
|
@@ -396,6 +402,24 @@ function LinkControl( { | |
| const isDisabled = ! valueHasChanges || currentInputIsEmpty; | ||
| const showSettings = !! settings?.length && isEditingLink && hasLinkValue; | ||
|
|
||
| const previewValue = useMemo( () => { | ||
| // There is a chance that the value is not yet set from the entity binding, so we use the preserved URL. | ||
| if ( | ||
| value?.kind === 'taxonomy' && | ||
| ! value?.url && | ||
| entityUrlFallbackRef.current | ||
| ) { | ||
| // combine the value prop with the preserved URL from the suggestion | ||
| return { | ||
| ...value, | ||
| url: entityUrlFallbackRef.current, | ||
| }; | ||
| } | ||
|
|
||
| // If we don't have a fallback URL, use the value prop. | ||
| return value; | ||
| }, [ value ] ); | ||
|
|
||
| return ( | ||
| <div | ||
| tabIndex={ -1 } | ||
|
|
@@ -487,8 +511,8 @@ function LinkControl( { | |
|
|
||
| { value && ! isEditingLink && ! isCreatingPage && ( | ||
| <LinkPreview | ||
| key={ value?.url } // force remount when URL changes to avoid race conditions for rich previews | ||
| value={ value } | ||
| key={ previewValue?.url } // force remount when URL changes to avoid race conditions for rich previews | ||
| value={ previewValue } | ||
| onEditClick={ () => setIsEditingLink( true ) } | ||
| hasRichPreviews={ hasRichPreviews } | ||
| hasUnlinkControl={ shownUnlinkControl } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,7 +30,7 @@ import { useState, useEffect, useRef, useCallback } from '@wordpress/element'; | |
| import { decodeEntities } from '@wordpress/html-entities'; | ||
| import { link as linkIcon, addSubmenu } from '@wordpress/icons'; | ||
| import { store as coreStore } from '@wordpress/core-data'; | ||
| import { useMergeRefs, usePrevious, useInstanceId } from '@wordpress/compose'; | ||
| import { useMergeRefs, useInstanceId } from '@wordpress/compose'; | ||
|
|
||
| /** | ||
| * Internal dependencies | ||
|
|
@@ -213,8 +213,11 @@ export default function NavigationLinkEdit( { | |
| const itemLabelPlaceholder = __( 'Add label…' ); | ||
| const ref = useRef(); | ||
| const linkUIref = useRef(); | ||
| const prevUrl = usePrevious( url ); | ||
| const isNewLink = useRef( ! url && ! metadata?.bindings?.url ); | ||
| // A link is "new" only if it has an undefined label | ||
| // After the link is created, even if no label is provided, it's set to an empty string. | ||
| const isNewLink = useRef( label === undefined ); | ||
|
Contributor
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. In #73368 we use a different method to check
Should we use that here?
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.
|
||
| // Track whether we should focus the submenu appender when closing the link UI | ||
| const shouldSelectSubmenuAppenderOnClose = useRef( false ); | ||
|
|
||
| const { | ||
| isAtMaxNesting, | ||
|
|
@@ -223,6 +226,7 @@ export default function NavigationLinkEdit( { | |
| hasChildren, | ||
| validateLinkStatus, | ||
| parentBlockClientId, | ||
| isSubmenu, | ||
| } = useSelect( | ||
| ( select ) => { | ||
| const { | ||
|
|
@@ -267,6 +271,7 @@ export default function NavigationLinkEdit( { | |
| hasChildren: !! getBlockCount( clientId ), | ||
| validateLinkStatus: enableLinkStatusValidation, | ||
| parentBlockClientId: parentBlockId, | ||
| isSubmenu: parentBlockName === 'core/navigation-submenu', | ||
|
Contributor
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. Just thinking... If we combine these two blocks into one we're going to need a different way to identify submenus...
Contributor
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. Likely an attribute. |
||
| }; | ||
| }, | ||
| [ clientId, maxNestingLevel ] | ||
|
|
@@ -314,7 +319,7 @@ export default function NavigationLinkEdit( { | |
| // If we leave focus on this block, then when we close the link without creating a link, focus will | ||
| // be lost during the new block selection process. | ||
| useEffect( () => { | ||
| if ( isNewLink.current && isSelected && ! url ) { | ||
| if ( isNewLink.current && isSelected ) { | ||
|
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. ! url check is unnecessary since ! url is already part of the |
||
| selectBlock( parentBlockClientId ); | ||
| } | ||
| }, [] ); // eslint-disable-line react-hooks/exhaustive-deps | ||
|
|
@@ -333,20 +338,46 @@ export default function NavigationLinkEdit( { | |
| transformToSubmenu, | ||
| ] ); | ||
|
|
||
| // If the LinkControl popover is open and the URL has changed, close the LinkControl and focus the label text. | ||
| // Handle link UI when a new link is created | ||
| useEffect( () => { | ||
| // We only want to do this when the URL has gone from nothing to a new URL AND the label looks like a URL | ||
| if ( | ||
| ! prevUrl && | ||
| url && | ||
| isLinkOpen && | ||
| isURL( prependHTTP( label ) ) && | ||
| /^.+\.[a-z]+/.test( label ) | ||
| ) { | ||
| // We know if a link was just created from our link UI if | ||
| // 1. isNewLink.current is true | ||
| // 2. url has a value | ||
| // 3. isLinkOpen is true | ||
| if ( ! isNewLink.current || ! url || ! isLinkOpen ) { | ||
| return; | ||
| } | ||
|
|
||
| // Ensure this only runs once | ||
| isNewLink.current = false; | ||
|
|
||
| // We just created a link and the block is now selected. | ||
| // If the label looks like a URL, focus and select the label text. | ||
| if ( isURL( prependHTTP( label ) ) && /^.+\.[a-z]+/.test( label ) ) { | ||
| // Focus and select the label text. | ||
| selectLabelText(); | ||
| } else { | ||
| // If the link was just created, we want to select the block so the inspector controls | ||
| // are accurate. | ||
| selectBlock( clientId, null ); | ||
|
|
||
| // Edge case: When the created link is the first child of a submenu, the focus will have | ||
| // originated from the add submenu toolbar button. In this case, we need to return focus | ||
| // to the submenu appender if the user closes the link ui using the keyboard. | ||
| // Check if this is the first and only child of a newly created submenu. | ||
| if ( isSubmenu ) { | ||
| const parentBlocks = getBlocks( parentBlockClientId ); | ||
| // If this is the only child, then this is a new submenu. | ||
| // Set the flag to select the submenu appender when the link ui is closed. | ||
| if ( | ||
| parentBlocks.length === 1 && | ||
| parentBlocks[ 0 ].clientId === clientId | ||
| ) { | ||
| shouldSelectSubmenuAppenderOnClose.current = true; | ||
| } | ||
| } | ||
| } | ||
| }, [ prevUrl, url, isLinkOpen, label ] ); | ||
| }, [ url, isLinkOpen, isNewLink, label ] ); | ||
|
|
||
| /** | ||
| * Focus the Link label text and select it. | ||
|
|
@@ -444,23 +475,20 @@ export default function NavigationLinkEdit( { | |
| } | ||
| ); | ||
|
|
||
| if ( | ||
| ! url || | ||
| const needsValidLink = | ||
| ( ! url && ! ( hasUrlBinding && isBoundEntityAvailable ) ) || | ||
|
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. The
Contributor
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. Can we combine this and the same code on line 493 into one const? |
||
| isInvalid || | ||
| isDraft || | ||
| ( hasUrlBinding && ! isBoundEntityAvailable ) | ||
| ) { | ||
| ( hasUrlBinding && ! isBoundEntityAvailable ); | ||
|
|
||
| if ( needsValidLink ) { | ||
| blockProps.onClick = () => { | ||
| setIsLinkOpen( true ); | ||
| }; | ||
| } | ||
|
|
||
| const classes = clsx( 'wp-block-navigation-item__content', { | ||
| 'wp-block-navigation-link__placeholder': | ||
| ! url || | ||
| isInvalid || | ||
| isDraft || | ||
| ( hasUrlBinding && ! isBoundEntityAvailable ), | ||
| 'wp-block-navigation-link__placeholder': needsValidLink, | ||
| } ); | ||
|
|
||
| const missingText = getMissingText( type ); | ||
|
|
@@ -589,9 +617,27 @@ export default function NavigationLinkEdit( { | |
| // Don't remove if binding exists (even if entity is unavailable) so user can fix it. | ||
| if ( ! url && ! hasUrlBinding ) { | ||
| onReplace( [] ); | ||
| } else if ( isNewLink.current ) { | ||
| // If we just created a new link, select it | ||
| selectBlock( clientId ); | ||
| return; | ||
| } | ||
|
|
||
| // Edge case: If this is the first child of a new submenu, focus the submenu's appender | ||
| if ( | ||
| shouldSelectSubmenuAppenderOnClose.current | ||
| ) { | ||
| shouldSelectSubmenuAppenderOnClose.current = false; | ||
|
|
||
| // The appender is the next sibling in the DOM after the current block | ||
| if ( | ||
| listItemRef.current?.nextElementSibling | ||
| ) { | ||
| const appenderButton = | ||
| listItemRef.current.nextElementSibling.querySelector( | ||
| '.block-editor-button-block-appender' | ||
| ); | ||
| if ( appenderButton ) { | ||
| appenderButton.focus(); | ||
| } | ||
| } | ||
| } | ||
| } } | ||
| anchor={ popoverAnchor } | ||
|
|
||
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.
Thanks for documenting this here, it will make it a lot easier to remove it when the time comes.