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

Force disable suggestions until URL field is dirty in Link Control #51354

Merged
merged 3 commits into from
Jun 13, 2023
Merged

Force disable suggestions until URL field is dirty in Link Control #51354

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1b1baef
Force disable suggestions if internal and current value match
getdave Jun 8, 2023
2eab077
Fix broken and outdated tests
getdave Jun 8, 2023
16e84e2
Add test coverage
getdave Jun 8, 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
1 change: 1 addition & 0 deletions packages/block-editor/src/components/link-control/search-input.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ const LinkControlSearchInput = forwardRef(
return (
<div className="block-editor-link-control__search-input-container">
<URLInput
disableSuggestions={ currentLink?.url === 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.

value here is the url (only) not the full value object.

__nextHasNoMarginBottom
label={ useLabel ? 'URL' : undefined }
className={ inputClasses }
Expand Down
91 changes: 40 additions & 51 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 @@ -582,44 +582,6 @@ describe( 'Searching for a link', () => {
expect( mockFetchSearchSuggestions ).not.toHaveBeenCalled();
} );

it.each( [
[ 'couldbeurlorentitysearchterm' ],
[ 'ThisCouldAlsoBeAValidURL' ],
] )(
'should display a URL suggestion as a default fallback for the search term "%s" which could potentially be a valid url.',
async ( searchTerm ) => {
const user = userEvent.setup();
render( <LinkControl /> );

// Search Input UI.
const searchInput = screen.getByRole( 'combobox', { name: 'URL' } );

// Simulate searching for a term.
await user.type( searchInput, searchTerm );

const searchResultElements = within(
await screen.findByRole( 'listbox', {
name: /Search results for.*/,
} )
).getAllByRole( 'option' );

const lastSearchResultItem =
searchResultElements[ searchResultElements.length - 1 ];

// We should see a search result for each of the expect search suggestions.
expect( searchResultElements ).toHaveLength(
fauxEntitySuggestions.length
);

// The URL search suggestion should not exist.
expect( lastSearchResultItem ).not.toHaveTextContent( searchTerm );
expect( lastSearchResultItem ).not.toHaveTextContent( 'URL' );
expect( lastSearchResultItem ).not.toHaveTextContent(
'Press ENTER to add this link'
);
Comment on lines -614 to -619
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These assertions were accidentally modified to .not which allowed the test to pass. However, the test itself no longer applies and makes sense because we actually don't want the URL to show up as a fallback anymore. We test against this elsewhere so it's safe to remove this one now.

}
);

it( 'should not display a URL suggestion as a default fallback when noURLSuggestion is passed.', async () => {
const user = userEvent.setup();
render( <LinkControl noURLSuggestion /> );
Expand Down Expand Up @@ -981,8 +943,6 @@ describe( 'Default search suggestions', () => {
const initialValue = fauxEntitySuggestions[ 0 ];
render( <LinkControl showInitialSuggestions value={ initialValue } /> );

expect( mockFetchSearchSuggestions ).not.toHaveBeenCalled();

// Click the "Edit/Change" button and check initial suggestions are not
// shown.
const currentLinkUI = screen.getByLabelText( 'Currently selected' );
Expand All @@ -992,23 +952,18 @@ describe( 'Default search suggestions', () => {
await user.click( currentLinkBtn );

const searchInput = screen.getByRole( 'combobox', { name: 'URL' } );

// Search input is set to the URL value.
expect( searchInput ).toHaveValue( initialValue.url );

// Focus the search input to display suggestions
await user.click( searchInput );

const searchResultElements = within(
await screen.findByRole( 'listbox', {
// Ensure no initial suggestions are shown.
expect(
screen.queryByRole( 'listbox', {
Comment on lines +959 to +961
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 diff isn't the best. Basically I'm changing the test to actually assert that no suggestions are shown. Previously we replied on focusing the input to trigger showing the results which had absolutely no merit that I can see.

Now this functionality is being removed anyway so it makes sense to fix and simplify the test.

name: /Search results for.*/,
} )
).getAllByRole( 'option' );

expect( searchResultElements ).toHaveLength( 4 );

expect( searchInput ).toHaveAttribute( 'aria-expanded', 'true' );
).not.toBeInTheDocument();

expect( mockFetchSearchSuggestions ).toHaveBeenCalledTimes( 1 );
expect( mockFetchSearchSuggestions ).not.toHaveBeenCalled();
} );

it( 'should display initial suggestions when input value is manually deleted', async () => {
Expand Down Expand Up @@ -1703,6 +1658,40 @@ describe( 'Selecting links', () => {

expect( mockFetchSearchSuggestions ).toHaveBeenCalledTimes( 1 );
} );

it( 'should not show search results on URL input focus when the URL has not changed', async () => {
const selectedLink = fauxEntitySuggestions[ 0 ];

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

// focus the search input
const searchInput = screen.getByRole( 'combobox', { name: 'URL' } );

fireEvent.focus( searchInput );

// check that the search results are not visible
expect(
screen.queryByRole( 'listbox', {
name: /Search results for.*/,
} )
).not.toBeInTheDocument();

// check that the mock fetch function was not called
expect( mockFetchSearchSuggestions ).not.toHaveBeenCalled();

// check that typing in the search input to make the value dirty
// does trigger search results
fireEvent.change( searchInput, { target: { value: 'changes' } } );

expect(
await screen.findByRole( 'listbox', {
name: /Search results for.*/,
} )
).toBeVisible();

// check the mock fetch function was called
expect( mockFetchSearchSuggestions ).toHaveBeenCalledTimes( 1 );
} );
} );
} );

Expand Down