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.

Sign up for GitHub

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

fix: 'open in new tab' option issue & multiple link control UX issues #50602

Closed
wants to merge 7 commits into from
Closed

fix: 'open in new tab' option issue & multiple link control UX issues #50602

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9a01925
fix: multiple link control isues
hz-tyfoon May 12, 2023
2b35bad
fix: clicking 'cancel' button still shows the updated change
hz-tyfoon May 15, 2023
c5eb7dc
reverted 'fix/item: 4' (see the description for details) as looks lik…
hz-tyfoon May 16, 2023
e3ce2c6
reverted 'fix/item: 5' (see the description for details) as looks lik…
hz-tyfoon May 16, 2023
ba870b2
revert / (slight refactor): kept the prop name 'onChange' rather than…
hz-tyfoon May 16, 2023
e33dffa
revert / (slight refactor): reverted back to old behaviour (selecting…
hz-tyfoon May 16, 2023
05924eb
revert: used 'value' rather than 'newValue' as per review
hz-tyfoon May 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 25 additions & 31 deletions packages/block-editor/src/components/link-control/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,17 +137,18 @@ function LinkControl( {
const isEndingEditWithFocus = useRef( false );

const [ settingsOpen, setSettingsOpen ] = useState( false );
const [ newValue, setNewValue ] = useState( value );

const [ internalUrlInputValue, setInternalUrlInputValue ] =
useInternalInputValue( value?.url || '' );
useInternalInputValue( newValue?.url || '' );

const [ internalTextInputValue, setInternalTextInputValue ] =
useInternalInputValue( value?.title || '' );
useInternalInputValue( newValue?.title || '' );

const [ isEditingLink, setIsEditingLink ] = useState(
forceIsEditingLink !== undefined
? forceIsEditingLink
: ! value || ! value.url
: ! newValue || ! newValue.url
);

const { createPage, isCreatingPage, errorMessage } =
Expand Down Expand Up @@ -192,7 +193,7 @@ function LinkControl( {
isEndingEditWithFocus.current = false;
}, [ isEditingLink, isCreatingPage ] );

const hasLinkValue = value?.url?.trim()?.length > 0;
const hasLinkValue = newValue?.url?.trim()?.length > 0;

/**
* Cancels editing state and marks that focus may need to be restored after
Expand All @@ -208,24 +209,23 @@ function LinkControl( {
};

const handleSelectSuggestion = ( updatedValue ) => {
onChange( {
const valueToApplyAndFormat = {
...updatedValue,
title: internalTextInputValue || updatedValue?.title,
} );
};
setNewValue( valueToApplyAndFormat );
onChange( valueToApplyAndFormat );
stopEditing();
};

const handleSubmit = () => {
if (
currentUrlInputValue !== value?.url ||
internalTextInputValue !== value?.title
) {
onChange( {
...value,
url: currentUrlInputValue,
title: internalTextInputValue,
} );
}
const valueToSubmit = {
...newValue,
url: currentUrlInputValue,
title: internalTextInputValue,
};
setNewValue( valueToSubmit );
onChange( valueToSubmit );
Copy link
Contributor

Choose a reason for hiding this comment

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

The original code didn't trigger onChange if nothing changed. Is this still true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review 😊 🙏 @draganescu
When submitted, Here I triggered onChange even the the 'title' & 'url' wasn't changed. I described the following in my PR's description that's I believe related to your question:

** 3) in 'handleSubmit' of 'index.js' removed the if check as looks like it's unnecessary. Because if user decide to submit without making any change to 'url' or 'title' there's no harm in still submitting it, also there's other change can happen like 'open in new tab settings' and user may just wanna submit that.

Copy link
Contributor Author

@hz-tyfoon hz-tyfoon May 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Just an update: I haven't found any issue caused by this so far. I'm assuming U haven't too.
But if U see any issue or potential issue/problem with this changes please feel free to share.

I'm not resolving this conversation at this moment. But looks so far it can be resolved if no issue found from your end.
Thanks again @draganescu . 😊

stopEditing();
};

Expand All @@ -240,19 +240,13 @@ function LinkControl( {
}
};

const resetInternalValues = () => {
setInternalUrlInputValue( value?.url );
setInternalTextInputValue( value?.title );
};

const handleCancel = ( event ) => {
event.preventDefault();
event.stopPropagation();

// Ensure that any unsubmitted input changes are reset.
resetInternalValues();
setNewValue( value );

if ( hasLinkValue ) {
if ( value?.url?.trim()?.length > 0 ) {
// If there is a link then exist editing mode and show preview.
stopEditing();
} else {
Expand All @@ -268,7 +262,7 @@ function LinkControl( {
const currentInputIsEmpty = ! currentUrlInputValue?.trim()?.length;

const shownUnlinkControl =
onRemove && value && ! isEditingLink && ! isCreatingPage;
onRemove && newValue && ! isEditingLink && ! isCreatingPage;

const showSettings = !! settings?.length;

Expand All @@ -277,7 +271,7 @@ function LinkControl( {
// See https://github.com/WordPress/gutenberg/pull/33849/#issuecomment-932194927.
const showTextControl = hasLinkValue && hasTextControl;

const isEditing = ( isEditingLink || ! value ) && ! isCreatingPage;
const isEditing = ( isEditingLink || ! newValue ) && ! isCreatingPage;

return (
<div
Expand All @@ -300,7 +294,7 @@ function LinkControl( {
} ) }
>
<LinkControlSearchInput
currentLink={ value }
currentLink={ newValue }
className="block-editor-link-control__field block-editor-link-control__search-input"
placeholder={ searchInputPlaceholder }
value={ currentUrlInputValue }
Expand Down Expand Up @@ -333,8 +327,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={ newValue?.url } // force remount when URL changes to avoid race conditions for rich previews
value={ newValue }
onEditClick={ () => setIsEditingLink( true ) }
hasRichPreviews={ hasRichPreviews }
hasUnlinkControl={ shownUnlinkControl }
Expand All @@ -356,9 +350,9 @@ function LinkControl( {
setInternalTextInputValue
}
handleSubmitWithEnter={ handleSubmitWithEnter }
value={ value }
value={ newValue }
settings={ settings }
onChange={ onChange }
onChange={ setNewValue }
/>
) }

Expand Down