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

DataView: only reset page if search has actually changed #61307

Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from
Open

DataView: only reset page if search has actually changed #61307

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
ad336ac
DataView: only reset page if search has actually changed
roo2 May 2, 2024
ed2d50a
add view to deps array
roo2 May 2, 2024
37d7756
add test
roo2 May 2, 2024
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
14 changes: 8 additions & 6 deletions packages/dataviews/src/search.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,14 @@ const Search = memo( function Search( { label, view, onChangeView } ) {
onChangeViewRef.current = onChangeView;
}, [ onChangeView ] );
useEffect( () => {
onChangeViewRef.current( {
...view,
page: 1,
search: debouncedSearch,
} );
}, [ debouncedSearch ] );
if ( debouncedSearch !== view.search ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to add view to the deps array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, yes.

Copy link
Member

Choose a reason for hiding this comment

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

In the demo you shared, pagination is reset after layout switch. I don't know how you're using DataViews in your code, but happy to take a look if there's some PR out there. This is not something that happens in the core component:

Gravacao.do.ecra.2024-05-02.as.14.22.32.mov

Copy link
Contributor

Choose a reason for hiding this comment

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

In pages we have preloaded the data (like Templates) this doesn't occur. In page though it does and it seems related to these lines of code: https://github.com/WordPress/gutenberg/blob/trunk/packages/edit-site/src/components/page-pages/index.js#L65. Did you have similar code in your explorations @roo2 ?

We need to check this either way. -cc @jorgefilipecosta

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ntsekouras I think the reason for the issue in our code is that we are updating the URL to include the filter, pagination params e.g. ?page=2 etc. This is causing the dataviews component to be re-rendered, which causes the line you mentioned to be hit several times and then the line I changed gets hit which would have reset the pagination before applying my change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can try to create a little demo or test case with re-rendering to prove this

onChangeViewRef.current( {
...view,
page: 1,
search: debouncedSearch,
} );
}
}, [ debouncedSearch, view ] );
const searchLabel = label || __( 'Search' );
return (
<SearchControl
Expand Down
50 changes: 50 additions & 0 deletions packages/dataviews/src/test/search.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/**
* External dependencies
*/
import { render } from '@testing-library/react';

/**
* Internal dependencies
*/
import Search from '../search.js';

describe( 'DataViews search component', () => {
const onChangeView = jest.fn();
it( 'should not reset page when first rendered', () => {
const view = {
search: '',
};
render(
<Search
label="search"
view={ view }
onChangeView={ onChangeView }
/>
);
expect( onChangeView ).toHaveBeenCalledTimes( 0 );
} );

it( 'should reset page if search term was changed', () => {
const view = {
search: '',
};
const { rerender } = render(
<Search
label="search"
view={ view }
onChangeView={ onChangeView }
/>
);
const viewNext = {
search: 'search term',
};
rerender(
<Search
label="search"
view={ viewNext }
onChangeView={ onChangeView }
/>
);
expect( onChangeView ).toHaveBeenCalledTimes( 1 );
} );
} );
Loading