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

Fix sorting a list using a <Datagrid> inside a <ReferenceManyField> #5094

Merged
merged 10 commits into from Aug 10, 2020

Conversation

Luwangel
Copy link
Contributor

@Luwangel Luwangel commented Jul 28, 2020

Fixes #5059

Several bugs are fixed:

  • Fix the switch between ASC and DESC orders inside <ReferenceManyField> and <ReferenceArrayField>
  • Fix sorting using another field than the default one or the first selected one

I introduced a new prop named field in the <DatagridHeaderCell> which contains the same data as the prop named sort which is now deprecated.

This renaming is motivated by the confusion introduced by two similar names (sort in <Datagrid> and sort in <DatagridHeaderCell>) containing different content. The first one contains an object { field: 'id', order: 'ASC' } named sort and the second one { sort: 'id', order: 'ASC' }.

Todo

  • Fix props used to memorize the <DatagridHeaderCell>
  • Fix the sorting issue of the <Datagrid>
  • Fix the updateSort method in the <Datagrid> to query the right order
  • Fix tests

Screenshots

Peek 28-07-2020 12-27

@Luwangel Luwangel added WIP Work In Progress breaking change labels Jul 28, 2020
@Luwangel Luwangel added RFR Ready For Review and removed WIP Work In Progress labels Jul 28, 2020
@fzaninotto
Copy link
Member

We should not introduce breaking changes at this stage. I suggest you fix the bug without renaming the arguments, and open an issue for the renaming and flag it with the breaking change tag. We'll address it for 4.0.

const newOrder =
currentSort.field === newField && currentSort.order === 'ASC'
? 'DESC'
: 'ASC';
Copy link
Member

Choose a reason for hiding this comment

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

The default sort order can de defined on a per field basis. You must rely on the data-order attribute.

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 value of the data-order attribute should be the same as the value of the currentSort.order because the <Datagrid> controls the <DatagridHeaderCell> which has no internal logic. But I made the change because it's more understandable.

Copy link
Member

Choose a reason for hiding this comment

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

In this new piece of code, if the user clicks on the header of a column that isn't yet used for sorting (thus currentSort.field !== newField), the sort order defaults to the ASC sort order. In the previous piece of code, the sort order defaulted to event.currentTarget.dataset.order. That's a breaking change.

You see, developers can customize the default sort order for a field using the sortByOrder prop on a Field - and that prop is stored in the header dataset. If you don't use that value, you're breaking the sortByOrder feature.

See https://marmelab.com/react-admin/List.html#specify-sort-order

packages/ra-ui-materialui/src/list/Datagrid.tsx Outdated Show resolved Hide resolved
const newOrder =
currentSort.field === newField && currentSort.order === 'ASC'
? 'DESC'
: 'ASC';
Copy link
Member

Choose a reason for hiding this comment

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

In this new piece of code, if the user clicks on the header of a column that isn't yet used for sorting (thus currentSort.field !== newField), the sort order defaults to the ASC sort order. In the previous piece of code, the sort order defaulted to event.currentTarget.dataset.order. That's a breaking change.

You see, developers can customize the default sort order for a field using the sortByOrder prop on a Field - and that prop is stored in the header dataset. If you don't use that value, you're breaking the sortByOrder feature.

See https://marmelab.com/react-admin/List.html#specify-sort-order

@fzaninotto fzaninotto removed the bug label Aug 9, 2020
@Luwangel Luwangel changed the base branch from next to master August 10, 2020 08:51
@fzaninotto fzaninotto merged commit 64aea11 into master Aug 10, 2020
@fzaninotto fzaninotto deleted the fix-sorting-datagrid branch August 10, 2020 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFR Ready For Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switching order problem in ReferenceManyField
2 participants