From 9a01925477c6f3ae3b721e014251f35c894e19c0 Mon Sep 17 00:00:00 2001 From: hz-tyfoon Date: Fri, 12 May 2023 22:01:04 +0600 Subject: [PATCH 1/7] fix: multiple link control isues MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit *** What I've Done 👇👇 *** ** 1) took a new state 'newValue'/'setNewValue' and took 'value' it's initial default. and replaced the usage of 'value' with this 'newValue' state everywhere in the index.js to make this state the source of truth. we'll need this state for couple of reasons and one being 'updating' the value using 'setNewValue' & another is replace 'onChange' call with 'setNewValue' or use both of them according to our need (more on this in the next steps to make it clear why this new state is taken, see the next steps for more clarity) ** 2) instead of passing the 'onChange' as prop to 'LinkControlSettingsDrawer', passed 'setNewValue' as prop. Because, the 'onChange' call makes the link popup modal to close without saving the other data properly. In 'settings-drawer.js' the receieved 'setNewValue' passed to the 'Settings' component as 'onChange' prop, this way toggling the 'open in new tab' won't close the modal & also will save the data in 'newValue' state properly. ** 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. Another thing done it 'handleSubmit' & that is called both 'setNewValue' and 'onChnage' to make sure the 'local state' (newValue) stays sync with the submitted data (otherwise there may be unwanted behaviour where 'link-control component' is used. i.e. 'button block') ** 4) imported the 'Settings' component inside 'index.js' and used it below the 'LinkPreview' component. used the neccessary css in 'style.scss' to properly style it. In this 'Settings' component's 'onChange' prop, passed a function & made sure the value gets called with both 'setNewValue' and 'onChnge'. This is done for the same reason of keeping the both state sync. This settings component showen there to keep the 'open in new tab option' visible when previewing the link (after initially any richtext is linked by selecting the text & then pasting the link ). This was done to keep it consistent with 'WordPress 6.2 core UI' and looks like many people find it better UX to keep the 'option' there. See these comments for more info: 1) https://github.com/WordPress/gutenberg/issues/49091\#issuecomment-1477057235 2) https://github.com/WordPress/gutenberg/issues/49091\#issuecomment-1477084979 (see the video in this comment for more clarity) 3) https://github.com/WordPress/gutenberg/issues/49091\#issuecomment-1477097292 ** 5) finally 2 extra works done related to keeping the settings panel visible when opening the modal as seems like it's a better UX to keep this option visible rather than keeping them hidden and then requiring 1 extra click to make them open. To achieve this set the initial default value to 'true' for 'settingsOpen' useState & removed the 'setSettingsOpen(false)' from 'stopEditing' function. *** Thanks 😊😊 *** --- .../src/components/link-control/index.js | 77 +++++++++++-------- .../link-control/settings-drawer.js | 4 +- .../src/components/link-control/style.scss | 14 +++- 3 files changed, 56 insertions(+), 39 deletions(-) diff --git a/packages/block-editor/src/components/link-control/index.js b/packages/block-editor/src/components/link-control/index.js index 142ffd6db1e503..4c40f171068e0f 100644 --- a/packages/block-editor/src/components/link-control/index.js +++ b/packages/block-editor/src/components/link-control/index.js @@ -20,6 +20,7 @@ import LinkControlSearchInput from './search-input'; import LinkPreview from './link-preview'; import useCreatePage from './use-create-page'; import useInternalInputValue from './use-internal-input-value'; +import Settings from './settings'; import { ViewerFill } from './viewer-slot'; import { DEFAULT_LINK_SETTINGS } from './constants'; @@ -136,18 +137,19 @@ function LinkControl( { const textInputRef = useRef(); const isEndingEditWithFocus = useRef( false ); - const [ settingsOpen, setSettingsOpen ] = useState( false ); + const [ settingsOpen, setSettingsOpen ] = useState( true ); + 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 } = @@ -192,7 +194,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 @@ -203,29 +205,24 @@ function LinkControl( { wrapperNode.current.ownerDocument.activeElement ); - setSettingsOpen( false ); setIsEditingLink( false ); }; const handleSelectSuggestion = ( updatedValue ) => { - onChange( { + setNewValue( { ...updatedValue, title: internalTextInputValue || updatedValue?.title, } ); - 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 ); stopEditing(); }; @@ -241,8 +238,8 @@ function LinkControl( { }; const resetInternalValues = () => { - setInternalUrlInputValue( value?.url ); - setInternalTextInputValue( value?.title ); + setInternalUrlInputValue( newValue?.url ); + setInternalTextInputValue( newValue?.title ); }; const handleCancel = ( event ) => { @@ -268,7 +265,7 @@ function LinkControl( { const currentInputIsEmpty = ! currentUrlInputValue?.trim()?.length; const shownUnlinkControl = - onRemove && value && ! isEditingLink && ! isCreatingPage; + onRemove && newValue && ! isEditingLink && ! isCreatingPage; const showSettings = !! settings?.length; @@ -277,7 +274,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 (
) } - { value && ! isEditingLink && ! isCreatingPage && ( - setIsEditingLink( true ) } - hasRichPreviews={ hasRichPreviews } - hasUnlinkControl={ shownUnlinkControl } - onRemove={ onRemove } - /> + { newValue && ! isEditingLink && ! isCreatingPage && ( + <> + setIsEditingLink( true ) } + hasRichPreviews={ hasRichPreviews } + hasUnlinkControl={ shownUnlinkControl } + onRemove={ onRemove } + /> +
+ { + setNewValue( val ); + onChange( val ); + } } + /> +
+ ) } { isEditing && ( @@ -356,9 +365,9 @@ function LinkControl( { setInternalTextInputValue } handleSubmitWithEnter={ handleSubmitWithEnter } - value={ value } + value={ newValue } settings={ settings } - onChange={ onChange } + setNewValue={ setNewValue } /> ) } diff --git a/packages/block-editor/src/components/link-control/settings-drawer.js b/packages/block-editor/src/components/link-control/settings-drawer.js index 0cb12f2c5904c6..7dc4d559d3cd9f 100644 --- a/packages/block-editor/src/components/link-control/settings-drawer.js +++ b/packages/block-editor/src/components/link-control/settings-drawer.js @@ -28,7 +28,7 @@ function LinkSettingsDrawer( { handleSubmitWithEnter, value, settings, - onChange, + setNewValue = () => {}, } ) { const prefersReducedMotion = useReducedMotion(); const MaybeAnimatePresence = prefersReducedMotion @@ -83,7 +83,7 @@ function LinkSettingsDrawer( { ) }
diff --git a/packages/block-editor/src/components/link-control/style.scss b/packages/block-editor/src/components/link-control/style.scss index e91b60cd97643e..d0a2ae001e2a90 100644 --- a/packages/block-editor/src/components/link-control/style.scss +++ b/packages/block-editor/src/components/link-control/style.scss @@ -462,13 +462,21 @@ $preview-image-height: 140px; } } -.block-editor-link-control__tools { +.block-editor-link-control__tools, +.block-editor-link-control__tools_in_preview { display: flex; - flex-wrap: wrap; align-items: center; - justify-content: space-between; margin: 0; padding: $grid-unit-20; +} + +.block-editor-link-control__tools_in_preview { + border-top: 1px solid #ddd; +} + +.block-editor-link-control__tools { + flex-wrap: wrap; + justify-content: space-between; // To hide the horizontal scrollbar on toggle. // Margin and padding are needed to prevent cutoff of the toggle button focus outline. From 2b35bad45d4437bd4c647ab53afd8a8a2af4caee Mon Sep 17 00:00:00 2001 From: hz-tyfoon Date: Mon, 15 May 2023 21:15:00 +0600 Subject: [PATCH 2/7] fix: clicking 'cancel' button still shows the updated change - fixed: when making some change after opening the 'link control modal' & without applying/submitting it, when the 'cancel' button is clicked it show's that the changes already applied even though it's cancelled (although the updated changes aren't really applied but, still it's a misleading UI so fixed it) --- .../block-editor/src/components/link-control/index.js | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/packages/block-editor/src/components/link-control/index.js b/packages/block-editor/src/components/link-control/index.js index 4c40f171068e0f..5eaecedcce2496 100644 --- a/packages/block-editor/src/components/link-control/index.js +++ b/packages/block-editor/src/components/link-control/index.js @@ -237,19 +237,13 @@ function LinkControl( { } }; - const resetInternalValues = () => { - setInternalUrlInputValue( newValue?.url ); - setInternalTextInputValue( newValue?.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 { From c5eb7dc4a452b387c0840edbe88102797f2a60be Mon Sep 17 00:00:00 2001 From: hz-tyfoon Date: Wed, 17 May 2023 02:22:18 +0600 Subject: [PATCH 3/7] reverted 'fix/item: 4' (see the description for details) as looks like it's better to handle it separately from 'PR 50602' MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit *** What was the 'fix/item: 4' that's reverted in this commit? here's what I wrote about it in my description 👇👇 *** "imported the 'Settings' component inside 'index.js' and used it below the 'LinkPreview' component. used the neccessary css in 'style.scss' to properly style it. In this 'Settings' component's 'onChange' prop, passed a function & made sure the value gets called with both 'setNewValue' and 'onChnge'. This is done for the same reason of keeping the both state sync. This settings component showen there to keep the 'open in new tab option' visible when previewing the link (after initially any richtext is linked by selecting the text & then pasting the link ). This was done to keep it consistent with 'WordPress 6.2 core UI' and looks like many people find it better UX to keep the 'option' there. See these comments for more info: this comment, also this comment with a video demonstration & this comment" --- .../src/components/link-control/index.js | 29 +++++-------------- .../src/components/link-control/style.scss | 14 ++------- 2 files changed, 11 insertions(+), 32 deletions(-) diff --git a/packages/block-editor/src/components/link-control/index.js b/packages/block-editor/src/components/link-control/index.js index 5eaecedcce2496..36543ee749684f 100644 --- a/packages/block-editor/src/components/link-control/index.js +++ b/packages/block-editor/src/components/link-control/index.js @@ -20,7 +20,6 @@ import LinkControlSearchInput from './search-input'; import LinkPreview from './link-preview'; import useCreatePage from './use-create-page'; import useInternalInputValue from './use-internal-input-value'; -import Settings from './settings'; import { ViewerFill } from './viewer-slot'; import { DEFAULT_LINK_SETTINGS } from './constants'; @@ -323,26 +322,14 @@ function LinkControl( { ) } { newValue && ! isEditingLink && ! isCreatingPage && ( - <> - setIsEditingLink( true ) } - hasRichPreviews={ hasRichPreviews } - hasUnlinkControl={ shownUnlinkControl } - onRemove={ onRemove } - /> -
- { - setNewValue( val ); - onChange( val ); - } } - /> -
- + setIsEditingLink( true ) } + hasRichPreviews={ hasRichPreviews } + hasUnlinkControl={ shownUnlinkControl } + onRemove={ onRemove } + /> ) } { isEditing && ( diff --git a/packages/block-editor/src/components/link-control/style.scss b/packages/block-editor/src/components/link-control/style.scss index d0a2ae001e2a90..e91b60cd97643e 100644 --- a/packages/block-editor/src/components/link-control/style.scss +++ b/packages/block-editor/src/components/link-control/style.scss @@ -462,21 +462,13 @@ $preview-image-height: 140px; } } -.block-editor-link-control__tools, -.block-editor-link-control__tools_in_preview { +.block-editor-link-control__tools { display: flex; + flex-wrap: wrap; align-items: center; + justify-content: space-between; margin: 0; padding: $grid-unit-20; -} - -.block-editor-link-control__tools_in_preview { - border-top: 1px solid #ddd; -} - -.block-editor-link-control__tools { - flex-wrap: wrap; - justify-content: space-between; // To hide the horizontal scrollbar on toggle. // Margin and padding are needed to prevent cutoff of the toggle button focus outline. From e3ce2c61ca25c23a93d145f431216642c78ba01a Mon Sep 17 00:00:00 2001 From: hz-tyfoon Date: Wed, 17 May 2023 02:29:10 +0600 Subject: [PATCH 4/7] reverted 'fix/item: 5' (see the description for details) as looks like it's better to handle it separately from 'PR 50602' MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit *** What was the 'fix/item: 5' that's removed in this commit? here's what I wrote about it in my description 👇👇 *** "finally 2 extra works done related to keeping the settings panel visible when opening the modal as seems like it's a better UX to keep this option visible rather than keeping them hidden and then requiring 1 extra click to make them open. To achieve this set the initial default value to 'true' for 'settingsOpen' useState & removed the 'setSettingsOpen(false)' from 'stopEditing' function." --- packages/block-editor/src/components/link-control/index.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/block-editor/src/components/link-control/index.js b/packages/block-editor/src/components/link-control/index.js index 36543ee749684f..711c5216bc70fb 100644 --- a/packages/block-editor/src/components/link-control/index.js +++ b/packages/block-editor/src/components/link-control/index.js @@ -136,7 +136,7 @@ function LinkControl( { const textInputRef = useRef(); const isEndingEditWithFocus = useRef( false ); - const [ settingsOpen, setSettingsOpen ] = useState( true ); + const [ settingsOpen, setSettingsOpen ] = useState( false ); const [ newValue, setNewValue ] = useState( value ); const [ internalUrlInputValue, setInternalUrlInputValue ] = @@ -204,6 +204,7 @@ function LinkControl( { wrapperNode.current.ownerDocument.activeElement ); + setSettingsOpen( false ); setIsEditingLink( false ); }; From ba870b2f6b60c03198e8c4f954da481d3f5e7820 Mon Sep 17 00:00:00 2001 From: hz-tyfoon Date: Wed, 17 May 2023 02:44:59 +0600 Subject: [PATCH 5/7] revert / (slight refactor): kept the prop name 'onChange' rather than 'setNewValue' but passed the 'setNewValue function' as suggested by dave I inititally renamed both the prop 'name and value' to 'setNewValue' so that it prevents any confusion being created (like, is the recieved 'onChange' prop in 'settings-drawe.js' is the same one which was recieved in the 'index.js'?) But, looks like that's not something to worry about at this moment. Looks like it's better to have that disscussion separately from this PR --- packages/block-editor/src/components/link-control/index.js | 2 +- .../src/components/link-control/settings-drawer.js | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/block-editor/src/components/link-control/index.js b/packages/block-editor/src/components/link-control/index.js index 711c5216bc70fb..8a48dfb3694575 100644 --- a/packages/block-editor/src/components/link-control/index.js +++ b/packages/block-editor/src/components/link-control/index.js @@ -349,7 +349,7 @@ function LinkControl( { handleSubmitWithEnter={ handleSubmitWithEnter } value={ newValue } settings={ settings } - setNewValue={ setNewValue } + onChange={ setNewValue } /> ) } diff --git a/packages/block-editor/src/components/link-control/settings-drawer.js b/packages/block-editor/src/components/link-control/settings-drawer.js index 7dc4d559d3cd9f..0cb12f2c5904c6 100644 --- a/packages/block-editor/src/components/link-control/settings-drawer.js +++ b/packages/block-editor/src/components/link-control/settings-drawer.js @@ -28,7 +28,7 @@ function LinkSettingsDrawer( { handleSubmitWithEnter, value, settings, - setNewValue = () => {}, + onChange, } ) { const prefersReducedMotion = useReducedMotion(); const MaybeAnimatePresence = prefersReducedMotion @@ -83,7 +83,7 @@ function LinkSettingsDrawer( { ) } From e33dffac60858fefbb298e808df8486f47052b8e Mon Sep 17 00:00:00 2001 From: hz-tyfoon Date: Wed, 17 May 2023 03:22:53 +0600 Subject: [PATCH 6/7] revert / (slight refactor): reverted back to old behaviour (selecting suggested links closes the modal) as per review Why? To keep concerns separate. Think It'd be better if this is disscussed separately & this PR shouldn't look to change this behaviour. more by dave here: https://github.com/WordPress/gutenberg/pull/50602\#discussion_r1194818569 --- packages/block-editor/src/components/link-control/index.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/packages/block-editor/src/components/link-control/index.js b/packages/block-editor/src/components/link-control/index.js index 8a48dfb3694575..289ae2c140f7e4 100644 --- a/packages/block-editor/src/components/link-control/index.js +++ b/packages/block-editor/src/components/link-control/index.js @@ -209,10 +209,13 @@ function LinkControl( { }; const handleSelectSuggestion = ( updatedValue ) => { - setNewValue( { + const valueToApplyAndFormat = { ...updatedValue, title: internalTextInputValue || updatedValue?.title, - } ); + }; + setNewValue( valueToApplyAndFormat ); + onChange( valueToApplyAndFormat ); + stopEditing(); }; const handleSubmit = () => { From 05924eb29bad88ce46cb9c5d10b6e581eff96c91 Mon Sep 17 00:00:00 2001 From: hz-tyfoon Date: Wed, 17 May 2023 03:31:46 +0600 Subject: [PATCH 7/7] revert: used 'value' rather than 'newValue' as per review why? We can disscuss which one is better in this case separately from this PR. see this https://github.com/WordPress/gutenberg/pull/50602\#discussion_r1194821669 --- packages/block-editor/src/components/link-control/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/block-editor/src/components/link-control/index.js b/packages/block-editor/src/components/link-control/index.js index 289ae2c140f7e4..0e4273f6715b10 100644 --- a/packages/block-editor/src/components/link-control/index.js +++ b/packages/block-editor/src/components/link-control/index.js @@ -325,7 +325,7 @@ function LinkControl( { ) } - { newValue && ! isEditingLink && ! isCreatingPage && ( + { value && ! isEditingLink && ! isCreatingPage && (