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

[Feature Request]: allow multiple columns for setDefaultSort() #1998

Open
Kell opened this issue Oct 15, 2024 · 1 comment
Open

[Feature Request]: allow multiple columns for setDefaultSort() #1998

Kell opened this issue Oct 15, 2024 · 1 comment

Comments

@Kell
Copy link

Kell commented Oct 15, 2024

Overview

Would be nice tobe able to add more than one column for default sorting.

Detailed explanation

maybe just allowing for array to set sorting $this->setDefaultSort(['sort' => 'asc', 'id' => 'asc' ]);

Notes

Currently one column is possible:
SortingConfiguration.php line 50:

 public function setDefaultSort(string $field, string $direction = 'asc'): self
    {
        $this->defaultSortColumn = $field;
        $this->defaultSortDirection = $direction;

        return $this;
    }

using multiple columns would be nice.
Probably just small check for array in the function applySorting() (WithSorting.php) and chaining orderBy() should do the trick?
current code in applySorting() around line 80:

$this->setBuilder($this->getBuilder()->orderBy($this->getDefaultSortColumn(), $this->getDefaultSortDirection()));
@lrljoe
Copy link
Collaborator

lrljoe commented Nov 23, 2024

So someone contributed this as a feature quite a while back, and it ended up breaking stuff, so we had to roll it back.

The problem with changing something as core as setDefaultSort, is there's 1.8m projects that either have/are using the package, and we can't break 1.8m sites just to add a small feature. So we have to get creative and add new methods in, and check which method to use.

This, along with the fact that we have around 90% of the code covered by tests, means it's never as simple as "small check for array" sadly. There's a lot of features sat waiting for me to write the tests and documentation!

I've noted it as a few people have requested it, if you want to create a functional PR, along with tests, and docs, that doesn't break existing functionality, then I'll be happy to review and merge it if it works.

Or if you get a few people to say "this is 100% what we want as the next feature, over and above anything else", then I'll look at it next.

Effectively, we'd have to add in a new configure() option to say "enableMultipleDefaultSorts", which would then ensure the appropriate methods are called.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants