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 9 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
99 changes: 80 additions & 19 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 Down Expand Up @@ -138,11 +139,45 @@ function LinkControl( {

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

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

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

const setInternalURLInputValue = ( nextValue ) => {
setInternalControlValue( {
...internalControlValue,
url: nextValue,
} );
};

const setInternalTextInputValue = ( nextValue ) => {
setInternalControlValue( {
...internalControlValue,
title: nextValue,
} );
};

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

// 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;
},
{}
);

setInternalControlValue( {
...internalControlValue,
...settingsUpdates,
} );
};
getdave marked this conversation as resolved.
Show resolved Hide resolved

const [ isEditingLink, setIsEditingLink ] = useState(
forceIsEditingLink !== undefined
Expand All @@ -160,6 +195,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 +245,49 @@ function LinkControl( {
};

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

// 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 +298,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 +319,8 @@ function LinkControl( {
onCancel?.();
};

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

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

Expand Down Expand Up @@ -306,7 +363,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 +408,16 @@ 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={ setInternalSettingValue }
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 changes means that any changes to settings are stored in internal state and not submitted until the user confirms with "Apply".

/>
) }

Expand All @@ -367,7 +426,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
59 changes: 55 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,57 @@ 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 ];

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 +2250,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 +2269,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 +2287,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
2 changes: 1 addition & 1 deletion packages/block-editor/src/components/link-control/use-internal-input-value.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { useState, useEffect } from '@wordpress/element';

export default function useInternalInputValue( value ) {
const [ internalInputValue, setInternalInputValue ] = useState(
value || ''
value || {}
);

// If the value prop changes, update the internal state.
Expand Down