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

Link Control require user to manually submit any changes #50668

Merged
merged 16 commits into from
May 25, 2023
Merged

Link Control require user to manually submit any changes #50668

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
2cdd638
Implement initial shallow checking
getdave May 16, 2023
5d4d1e8
Don’t assyne value when comparing value changes
getdave May 16, 2023
1dffb00
Fix recrusive re-renders in hook
getdave May 16, 2023
42913c2
Fix ENTER submission bug
getdave May 16, 2023
b74ca1a
Require settings changes to be “Applied”
getdave May 16, 2023
e95a4d9
Refactor for readability and tidy
getdave May 16, 2023
c0fcf91
Improve test naming
getdave May 16, 2023
a0a54b0
Add test for new UX
getdave May 16, 2023
d8006a3
Improve fetching of settings
getdave May 16, 2023
dea11dd
Rename hook to convey new purpose
getdave May 16, 2023
77a9a6e
Improve test coverage of test
getdave May 16, 2023
48a708b
Fix tab stops now Apply is disabled when there are no changes
getdave May 25, 2023
f09df37
Wait for settings drawer to open
getdave May 25, 2023
1afabdd
Extract retrival of settings keys
getdave May 25, 2023
4f16173
Move setters to hook
getdave May 25, 2023
3e6f682
Make API clearer to consumers of hook
getdave May 25, 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
74 changes: 54 additions & 20 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 @@ -11,6 +11,7 @@ import { __ } from '@wordpress/i18n';
import { useRef, useState, useEffect } from '@wordpress/element';
import { focus } from '@wordpress/dom';
import { ENTER } from '@wordpress/keycodes';
import { isShallowEqualObjects } from '@wordpress/is-shallow-equal';

/**
* Internal dependencies
Expand All @@ -19,7 +20,7 @@ import LinkControlSettingsDrawer from './settings-drawer';
import LinkControlSearchInput from './search-input';
import LinkPreview from './link-preview';
import useCreatePage from './use-create-page';
import useInternalInputValue from './use-internal-input-value';
import useInternalValue from './use-internal-value';
import { ViewerFill } from './viewer-slot';
import { DEFAULT_LINK_SETTINGS } from './constants';

Expand Down Expand Up @@ -136,13 +137,20 @@ function LinkControl( {
const textInputRef = useRef();
const isEndingEditWithFocus = useRef( false );

const settingsKeys = settings.map( ( { id } ) => id );

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

const [ internalUrlInputValue, setInternalUrlInputValue ] =
useInternalInputValue( value?.url || '' );
const [
internalControlValue,
setInternalControlValue,
setInternalURLInputValue,
setInternalTextInputValue,
createSetInternalSettingValueHandler,
] = useInternalValue( value );

const [ internalTextInputValue, setInternalTextInputValue ] =
useInternalInputValue( value?.title || '' );
const valueHasChanges =
value && ! isShallowEqualObjects( internalControlValue, value );

const [ isEditingLink, setIsEditingLink ] = useState(
forceIsEditingLink !== undefined
Expand All @@ -160,6 +168,8 @@ function LinkControl( {
) {
setIsEditingLink( forceIsEditingLink );
}
// Todo: bug if the missing dep is introduced. Will need a fix.
// eslint-disable-next-line react-hooks/exhaustive-deps
Comment on lines +171 to +172
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is unrelated and is caused by the enabling of the relevant eslint rule. Fixing the code to conform causes a bug so I don't want to fix this now.

}, [ forceIsEditingLink ] );

useEffect( () => {
Expand Down Expand Up @@ -208,29 +218,47 @@ function LinkControl( {
};

const handleSelectSuggestion = ( updatedValue ) => {
// Suggestions may contains "settings" values (e.g. `opensInNewTab`)
// which should not overide any existing settings values set by the
// user. This filters out any settings values from the suggestion.
const nonSettingsChanges = Object.keys( updatedValue ).reduce(
Copy link
Contributor

Choose a reason for hiding this comment

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

You have a block of code very similar to this above too. Would moving it to the hook mean that we can combine them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't require a hook just a function but yes 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually in future this code won't be necessary so we can ignore it for now and clean up post-refactor if it's still around.

( acc, key ) => {
if ( ! settingsKeys.includes( key ) ) {
acc[ key ] = updatedValue[ key ];
}
return acc;
},
{}
);

onChange( {
...updatedValue,
title: internalTextInputValue || updatedValue?.title,
...internalControlValue,
...nonSettingsChanges,
// As title is not a setting, it must be manually applied
// in such a way as to preserve the users changes over
// any "title" value provided by the "suggestion".
title: internalControlValue?.title || updatedValue?.title,
} );

stopEditing();
};

const handleSubmit = () => {
if (
currentUrlInputValue !== value?.url ||
internalTextInputValue !== value?.title
) {
if ( valueHasChanges ) {
Comment on lines -219 to +247
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of testing for individual properties of the value, this uses a shallow comparison to check if the external (controlled) and internal values are different. If there is a difference then the internal value has changed and thus we need to submit those changes.

// Submit the original value with new stored values applied
// on top. URL is a special case as it may also be a prop.
onChange( {
...value,
...internalControlValue,
url: currentUrlInputValue,
title: internalTextInputValue,
} );
}
stopEditing();
};

const handleSubmitWithEnter = ( event ) => {
const { keyCode } = event;

if (
keyCode === ENTER &&
! currentInputIsEmpty // Disallow submitting empty values.
Expand All @@ -241,8 +269,7 @@ function LinkControl( {
};

const resetInternalValues = () => {
setInternalUrlInputValue( value?.url );
setInternalTextInputValue( value?.title );
setInternalControlValue( value );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of resetting individual properties, "reset" now just means "sync back to whatever value was/is"

};

const handleCancel = ( event ) => {
Expand All @@ -263,7 +290,8 @@ function LinkControl( {
onCancel?.();
};

const currentUrlInputValue = propInputValue || internalUrlInputValue;
const currentUrlInputValue =
propInputValue || internalControlValue?.url || '';

const currentInputIsEmpty = ! currentUrlInputValue?.trim()?.length;

Expand Down Expand Up @@ -306,7 +334,7 @@ function LinkControl( {
value={ currentUrlInputValue }
withCreateSuggestion={ withCreateSuggestion }
onCreateSuggestion={ createPage }
onChange={ setInternalUrlInputValue }
onChange={ setInternalURLInputValue }
onSelect={ handleSelectSuggestion }
showInitialSuggestions={ showInitialSuggestions }
allowDirectEntry={ ! noDirectEntry }
Expand Down Expand Up @@ -351,14 +379,18 @@ function LinkControl( {
showTextControl={ showTextControl }
showSettings={ showSettings }
textInputRef={ textInputRef }
internalTextInputValue={ internalTextInputValue }
internalTextInputValue={
internalControlValue?.title
}
setInternalTextInputValue={
setInternalTextInputValue
}
handleSubmitWithEnter={ handleSubmitWithEnter }
value={ value }
value={ internalControlValue }
settings={ settings }
onChange={ onChange }
onChange={ createSetInternalSettingValueHandler(
settingsKeys
) }
/>
) }

Expand All @@ -367,7 +399,9 @@ function LinkControl( {
variant="primary"
onClick={ handleSubmit }
className="block-editor-link-control__search-submit"
disabled={ currentInputIsEmpty } // Disallow submitting empty values.
disabled={
! valueHasChanges || currentInputIsEmpty
}
Comment on lines +402 to +404
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This means the Apply button will not be active until changes to the internal value are detected.

>
{ __( 'Apply' ) }
</Button>
Expand Down
65 changes: 61 additions & 4 deletions packages/block-editor/src/components/link-control/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1784,6 +1784,63 @@ describe( 'Addition Settings UI', () => {
} )
).toBeChecked();
} );

it( 'should require settings changes to be submitted/applied', async () => {
const user = userEvent.setup();

const mockOnChange = jest.fn();

const selectedLink = {
...fauxEntitySuggestions[ 0 ],
// Including a setting here helps to assert on a potential bug
// whereby settings on the suggestion override the current (internal)
// settings values set by the user in the UI.
opensInNewTab: false,
};

render(
<LinkControl
value={ selectedLink }
forceIsEditingLink
hasTextControl
onChange={ mockOnChange }
/>
);

// check that the "Apply" button is disabled by default.
const submitButton = screen.queryByRole( 'button', {
name: 'Apply',
} );

expect( submitButton ).toBeDisabled();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When there are no changes Submit should be disabled.


await toggleSettingsDrawer( user );

const opensInNewTabToggle = screen.queryByRole( 'checkbox', {
name: 'Open in new tab',
} );

// toggle the checkbox
await user.click( opensInNewTabToggle );

// Check settings are **not** directly submitted
// which would trigger the onChange handler.
expect( mockOnChange ).not.toHaveBeenCalled();
Comment on lines +1824 to +1828
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously changing a setting automatically "submitted" the value (i.e. calls onChange). This asserts that this is no longer the case.


// Check Apply button is now enabled because changes
// have been detected.
expect( submitButton ).toBeEnabled();

// Submit the changed setting value using the Apply button
await user.click( submitButton );

// Assert the value is updated.
expect( mockOnChange ).toHaveBeenCalledWith(
expect.objectContaining( {
opensInNewTab: true,
} )
);
Comment on lines +1832 to +1842
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now a user is required to manually submit any changes to the settings.

} );
} );

describe( 'Post types', () => {
Expand Down Expand Up @@ -2199,7 +2256,7 @@ describe( 'Controlling link title text', () => {

it( 'should allow `ENTER` keypress within the text field to trigger submission of value', async () => {
const user = userEvent.setup();
const textValue = 'My new text value';
const newTextValue = 'My new text value';
const mockOnChange = jest.fn();

render(
Expand All @@ -2218,14 +2275,14 @@ describe( 'Controlling link title text', () => {
expect( textInput ).toBeVisible();

await user.clear( textInput );
await user.keyboard( textValue );
await user.keyboard( newTextValue );

// Attempt to submit the empty search value in the input.
triggerEnter( textInput );

expect( mockOnChange ).toHaveBeenCalledWith(
expect.objectContaining( {
title: textValue,
title: newTextValue,
url: selectedLink.url,
} )
);
Expand All @@ -2236,7 +2293,7 @@ describe( 'Controlling link title text', () => {
).not.toBeInTheDocument();
} );

it( 'should reset state on value change', async () => {
it( 'should reset state upon controlled value change', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unrelated change but it helps improve the meaning of the test.

const user = userEvent.setup();
const textValue = 'My new text value';
const mockOnChange = jest.fn();
Expand Down
23 changes: 0 additions & 23 deletions packages/block-editor/src/components/link-control/use-internal-input-value.js
View file
Open in desktop

This file was deleted.

60 changes: 60 additions & 0 deletions packages/block-editor/src/components/link-control/use-internal-value.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
/**
* WordPress dependencies
*/
import { useState, useEffect } from '@wordpress/element';

export default function useInternalValue( value ) {
const [ internalValue, setInternalValue ] = useState( value || {} );

// If the value prop changes, update the internal state.
useEffect( () => {
setInternalValue( ( prevValue ) => {
if ( value && value !== prevValue ) {
return value;
}

return prevValue;
} );
}, [ value ] );
Comment on lines +9 to +18
Copy link
Member

Choose a reason for hiding this comment

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

Do you know if this sync hook is still needed? Now that value is an object; the synchronization can cause the content loss.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So we originally introduced the pattern to fix an issue whereby programmatically updating value didn't update the UI.

@Mamaduka The example is codified in one of the unit tests. Basically if you create x2 separate and different links in a paragraph block and then click between them the UI will reflect the value of that link. Previous to this sync you would continue to see the previous link's value in the UI even if you clicked on another link.

You won't be able to replicate any more because we implemented another fix which passes a key prop to the LinkControl in rich text to force a unique component instance per link instance.

I think we could look to remove the sync. Perhaps a PoC PR is a good place to start. I'll spin up. I've never liked this code and I agree is prone to error.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, Dave. Let’s continue discussion in the new issue.


const setInternalURLInputValue = ( nextValue ) => {
setInternalValue( {
...internalValue,
url: nextValue,
} );
};

const setInternalTextInputValue = ( nextValue ) => {
setInternalValue( {
...internalValue,
title: nextValue,
} );
};

const createSetInternalSettingValueHandler =
( settingsKeys ) => ( nextValue ) => {
// Only apply settings values which are defined in the settings prop.
const settingsUpdates = Object.keys( nextValue ).reduce(
( acc, key ) => {
if ( settingsKeys.includes( key ) ) {
acc[ key ] = nextValue[ key ];
}
return acc;
},
{}
);

setInternalValue( {
...internalValue,
...settingsUpdates,
} );
};

return [
internalValue,
setInternalValue,
setInternalURLInputValue,
setInternalTextInputValue,
createSetInternalSettingValueHandler,
];
}
5 changes: 4 additions & 1 deletion packages/e2e-tests/specs/editor/various/links.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -792,8 +792,11 @@ describe( 'Links', () => {
);
await settingsToggle.click();

// Wait for settings to open.
await page.waitForXPath( `//label[text()='Open in new tab']` );

// Move focus back to RichText for the underlying link.
await pressKeyTimes( 'Tab', 5 );
await pressKeyTimes( 'Tab', 4 );

// Make a selection within the RichText.
await pressKeyWithModifier( 'shift', 'ArrowRight' );
Expand Down