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

DataViews: Remove redundant setSelection prop #63648

Merged
merged 3 commits into from
Jul 17, 2024
Merged

DataViews: Remove redundant setSelection prop #63648

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
d83ca04
DataViews: Remove redundant setSelection prop
youknowriad Jul 17, 2024
d680c7d
Fix url changes
youknowriad Jul 17, 2024
42d292d
Clarify changelog
youknowriad Jul 17, 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
3 changes: 2 additions & 1 deletion packages/dataviews/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@

### Breaking Changes

- `onSelectionChange` has been renamed to `onChangeSelection`.
- `onSelectionChange` prop has been renamed to `onChangeSelection` and its argument has been updated to be a list of ids.
- `setSelection` prop has been removed. Please use `onChangeSelection` instead.

## 3.0.0 (2024-07-10)

Expand Down
25 changes: 10 additions & 15 deletions packages/dataviews/src/dataviews.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ import type {
ViewBaseProps,
SupportedLayouts,
} from './types';
import type { SetSelection, SelectionOrUpdater } from './private-types';
import type { SelectionOrUpdater } from './private-types';

type ItemWithId = { id: string };

Expand All @@ -50,16 +50,13 @@ type DataViewsProps< Item > = {
};
defaultLayouts: SupportedLayouts;
selection?: string[];
setSelection?: SetSelection;
onChangeSelection?: ( items: Item[] ) => void;
onChangeSelection?: ( items: string[] ) => void;
} & ( Item extends ItemWithId
? { getItemId?: ( item: Item ) => string }
: { getItemId: ( item: Item ) => string } );

const defaultGetItemId = ( item: ItemWithId ) => item.id;

const defaultOnChangeSelection = () => {};

export default function DataViews< Item >( {
view,
onChangeView,
Expand All @@ -73,25 +70,23 @@ export default function DataViews< Item >( {
paginationInfo,
defaultLayouts,
selection: selectionProperty,
setSelection: setSelectionProperty,
onChangeSelection = defaultOnChangeSelection,
onChangeSelection,
}: DataViewsProps< Item > ) {
const [ selectionState, setSelectionState ] = useState< string[] >( [] );
const isUncontrolled =
selectionProperty === undefined || setSelectionProperty === undefined;
selectionProperty === undefined || onChangeSelection === undefined;
const selection = isUncontrolled ? selectionState : selectionProperty;
const setSelection = isUncontrolled
? setSelectionState
: setSelectionProperty;
const [ openedFilter, setOpenedFilter ] = useState< string | null >( null );

function setSelectionWithChange( value: SelectionOrUpdater ) {
const newValue =
typeof value === 'function' ? value( selection ) : value;
onChangeSelection(
data.filter( ( item ) => newValue.includes( getItemId( item ) ) )
);
Copy link
Member

Choose a reason for hiding this comment

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

Aren't we now passing the items? Previously we very extracting the ids and now we don't?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's the opposite, we're now passing "strings" while we previously were passing "items".

The extra logic here is to filter items that are in the selection but not in the data. The problem though is that this filtering doesn't really make sense when calling onChange since we're calling it from the "inside" meaning we know the ids are in the data already.

Copy link
Member

Choose a reason for hiding this comment

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

Oh right, I actually changed it internally to IDs before, I forgot :D

return setSelection( value );
if ( ! isUncontrolled ) {
setSelectionState( newValue );
}
if ( onChangeSelection ) {
onChangeSelection( newValue );
Copy link
Member

Choose a reason for hiding this comment

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

Can't we just do something like this now:

const setSelectionWithChange = onChangeSelection || setSelectionState

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no we can't, because sometimes you want the selection to be uncontrolled (you don't pass any selection prop) but you want to be notified about selection changes so you provide a onChangeSelection prop.

Copy link
Member

Choose a reason for hiding this comment

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

When do we want this? Can't we always force it to be controlled if you want to be notified?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For me, it's a good idea to support this, just like regular inputs or any raw form element in React. You can pass the value to make it controlled otherwise you can use onChange to be notified.

Copy link
Member

Choose a reason for hiding this comment

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

Doesn't it log a warning in that case about an uncontrolled 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.

No, it logs a warning only when you switch from controlled to uncontrolled (or the opposite)

}
}

const ViewComponent = VIEW_LAYOUTS.find( ( v ) => v.type === view.type )
Expand Down
4 changes: 2 additions & 2 deletions packages/edit-site/src/components/page-templates/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,10 +258,11 @@ export default function PageTemplates() {
const history = useHistory();
const onChangeSelection = useCallback(
( items ) => {
setSelection( items );
if ( view?.type === LAYOUT_LIST ) {
history.push( {
...params,
postId: items.length === 1 ? items[ 0 ].id : undefined,
postId: items.length === 1 ? items[ 0 ] : undefined,
} );
}
},
Expand Down Expand Up @@ -374,7 +375,6 @@ export default function PageTemplates() {
onChangeView={ onChangeView }
onChangeSelection={ onChangeSelection }
selection={ selection }
setSelection={ setSelection }
defaultLayouts={ defaultLayouts }
/>
</Page>
Expand Down
4 changes: 2 additions & 2 deletions packages/edit-site/src/components/post-list/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,14 +284,15 @@ export default function PostList( { postType } ) {
const [ selection, setSelection ] = useState( [ postId ] );
const onChangeSelection = useCallback(
( items ) => {
setSelection( items );
const { params } = history.getLocationWithParams();
if (
( params.isCustom ?? 'false' ) === 'false' &&
view?.type === LAYOUT_LIST
) {
history.push( {
...params,
postId: items.length === 1 ? items[ 0 ].id : undefined,
postId: items.length === 1 ? items[ 0 ] : undefined,
} );
}
},
Expand Down Expand Up @@ -611,7 +612,6 @@ export default function PostList( { postType } ) {
view={ view }
onChangeView={ setView }
selection={ selection }
setSelection={ setSelection }
onChangeSelection={ onChangeSelection }
getItemId={ getItemId }
defaultLayouts={ defaultLayouts }
Expand Down
Loading