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.

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

DataViews: Fix filters lost when switching layouts #67740

Merged
merged 2 commits into from
Dec 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
90 changes: 48 additions & 42 deletions packages/edit-site/src/components/post-list/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import { DataViews, filterSortAndPaginate } from '@wordpress/dataviews';
import { privateApis as editorPrivateApis } from '@wordpress/editor';
import { __ } from '@wordpress/i18n';
import { drawerRight } from '@wordpress/icons';
import { usePrevious } from '@wordpress/compose';
import { useEvent, usePrevious } from '@wordpress/compose';
import { addQueryArgs } from '@wordpress/url';

/**
Expand Down Expand Up @@ -112,53 +112,50 @@ function useView( postType ) {
};
} );

const setViewWithUrlUpdate = useCallback(
( newView ) => {
if ( newView.type === LAYOUT_LIST && ! layout ) {
// Skip updating the layout URL param if
// it is not present and the newView.type is LAYOUT_LIST.
} else if ( newView.type !== layout ) {
history.navigate(
addQueryArgs( path, {
layout: newView.type,
} )
);
}
const setViewWithUrlUpdate = useEvent( ( newView ) => {
setView( newView );

setView( newView );
if ( isCustom === 'true' && editedEntityRecord?.id ) {
editEntityRecord(
'postType',
'wp_dataviews',
editedEntityRecord?.id,
{
content: JSON.stringify( newView ),
}
);
}

if ( isCustom === 'true' && editedEntityRecord?.id ) {
editEntityRecord(
'postType',
'wp_dataviews',
editedEntityRecord?.id,
{
content: JSON.stringify( newView ),
}
);
}
},
[
history,
isCustom,
editEntityRecord,
editedEntityRecord?.id,
layout,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The bug was the presence of "layout" in this hook which caused the "view" to reset even if we didn't want it to.
I refactored the code to use useEvent which means these depenedencies are not needed unless we really want them.

I think it's easier to read now.

path,
]
);
const currentUrlLayout = layout ?? LAYOUT_LIST;
if ( newView.type !== currentUrlLayout ) {
history.navigate(
addQueryArgs( path, {
layout: newView.type,
} )
);
}
} );

// When layout URL param changes, update the view type
// without affecting any other config.
const onUrlLayoutChange = useEvent( () => {
setView( ( prevView ) => {
const layoutToApply = layout ?? LAYOUT_LIST;
if ( layoutToApply === prevView.type ) {
return prevView;
}
return {
...prevView,
type: layout ?? LAYOUT_LIST,
};
} );
} );
useEffect( () => {
setView( ( prevView ) => ( {
...prevView,
type: layout ?? LAYOUT_LIST,
} ) );
}, [ layout ] );
onUrlLayoutChange();
}, [ onUrlLayoutChange, layout ] );

// When activeView or isCustom URL parameters change, reset the view.
useEffect( () => {
const onUrlActiveViewChange = useEvent( () => {
let newView;
if ( isCustom === 'true' ) {
newView = getCustomView( editedEntityRecord );
Expand All @@ -173,9 +170,18 @@ function useView( postType ) {
type,
} );
}
}, [ activeView, isCustom, layout, defaultViews, editedEntityRecord ] );
} );
useEffect( () => {
onUrlActiveViewChange();
}, [
onUrlActiveViewChange,
activeView,
isCustom,
defaultViews,
editedEntityRecord,
] );

return [ view, setViewWithUrlUpdate, setViewWithUrlUpdate ];
return [ view, setViewWithUrlUpdate ];
}

const DEFAULT_STATUSES = 'draft,future,pending,private,publish'; // All but 'trash'.
Expand Down
56 changes: 56 additions & 0 deletions test/e2e/specs/site-editor/page-list.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
/**
* WordPress dependencies
*/
const { test, expect } = require( '@wordpress/e2e-test-utils-playwright' );

test.describe( 'Page List', () => {
test.beforeAll( async ( { requestUtils } ) => {
// Activate a theme with permissions to access the site editor.
await requestUtils.activateTheme( 'emptytheme' );
await requestUtils.createPage( {
title: 'Privacy Policy',
status: 'publish',
} );
await requestUtils.createPage( {
title: 'Sample Page',
status: 'publish',
} );
} );

test.afterAll( async ( { requestUtils } ) => {
// Go back to the default theme.
await Promise.all( [
requestUtils.activateTheme( 'twentytwentyone' ),
requestUtils.deleteAllPages(),
] );
} );

test.beforeEach( async ( { admin, page } ) => {
// Go to the pages page, as it has the list layout enabled by default.
await admin.visitSiteEditor();
await page.getByRole( 'button', { name: 'Pages' } ).click();
} );

test( 'Persists filter/search when switching layout', async ( {
page,
} ) => {
// Search pages
await page
.getByRole( 'searchbox', { name: 'Search' } )
.fill( 'Privacy' );

// Switch layout
await page.getByRole( 'button', { name: 'Layout' } ).click();
await page.getByRole( 'menuitemradio', { name: 'Table' } ).click();

// Confirm the table is visible
await expect( page.getByRole( 'table' ) ).toContainText(
'Privacy Policy'
);

// The search should still contain the search term
await expect(
page.getByRole( 'searchbox', { name: 'Search' } )
).toHaveValue( 'Privacy' );
} );
} );
Loading