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

Jump to bottom

FIX Respect default sort array in GridFieldOrderableRows #349

Closed
wants to merge 2 commits into from
Closed

FIX Respect default sort array in GridFieldOrderableRows #349

wants to merge 2 commits into from

Conversation

vinstah
Copy link

@vinstah vinstah commented Aug 17, 2022

No description provided.

@vinstah
Copy link
Author

vinstah commented Aug 19, 2022

#324 also touches on this

GuySartorelli
GuySartorelli requested changes Aug 31, 2022
View reviewed changes
Copy link
Collaborator

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

If the default sort is an array, that means there are multiple fields in cascading priority by which the list should be sorted. We should respect that instead of just grabbing the top one and discarding the rest.

@GuySartorelli GuySartorelli changed the title array to string conversion line 370 GridFieldOrderableRows FIX Respect default sort array in GridFieldOrderableRows Aug 31, 2022
@GuySartorelli GuySartorelli linked an issue Aug 31, 2022 that may be closed by this pull request
GridFieldOrderableRows does not support secondary or default sort fields. #324
Closed
@GuySartorelli GuySartorelli mentioned this pull request Aug 31, 2022
Closed
@GuySartorelli
Copy link
Collaborator

@vinstah are you interested in making the change I've requested?

bclegaspi
bclegaspi approved these changes Oct 31, 2022
View reviewed changes
Copy link

@bclegaspi bclegaspi left a comment

Choose a reason for hiding this comment

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

Can these be merged already please? thank you

@GuySartorelli
Copy link
Collaborator

@bclegaspi There are requested changes which have not been made. The PR will not be merged in its current state. If @vinstah makes the requested changes it can be merged - otherwise if you want to create a new PR with the requested changes, that new PR can be reviewed and merged if it resolves the issue and complies with best practices.

@vinstah
Copy link
Author

vinstah commented Nov 3, 2022

Hi have been a bit busy lately, have been thinking about the change but just haven't implemented yet, hoping to get some free time in the next couple of weeks

@vinstah vinstah requested a review from GuySartorelli November 4, 2022 02:24
@gurucomkz gurucomkz mentioned this pull request Dec 22, 2022
Closed
@GuySartorelli GuySartorelli mentioned this pull request Mar 19, 2023
Closed
@emteknetnz
Copy link
Collaborator

Closed in favor of #361

@emteknetnz emteknetnz closed this Apr 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bclegaspi bclegaspi bclegaspi approved these changes

@GuySartorelli GuySartorelli Awaiting requested review from GuySartorelli

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

GridFieldOrderableRows does not support secondary or default sort fields.
4 participants
@vinstah @GuySartorelli @emteknetnz @bclegaspi