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

Add multiple sort field support in sort_link #438

Merged
merged 2 commits into from
Oct 2, 2014
Merged

Add multiple sort field support in sort_link #438

merged 2 commits into from
Oct 2, 2014

Conversation

caleb
Copy link
Contributor

@caleb caleb commented Oct 2, 2014

Let me know what you think of this patch. I would love to see this included in ransack, since this is a problem I run into frequently.

This patch allows users to sort on multiple fields with the sort_link
helper.

To specify sorting on multiple fields:

sort_link(:kind, [:kind, 'name asc'])

This will create a sort link that sorts first by kind, and then by
name. The first :kind parameter ensures that the link generated
shows the sort status of the kind field.

When you specify a sort direction in the sort fields array, the
direction is locked to that direction. In the above example, clicking
the resulting link would toggle sorting of the kind field, but the name
field would always sort ascending.

Also added was the ability to specify multiple default_order fields
with a hash:

sort_link(:kind, [:kind, :name], :default_order => { :name => 'asc', :kind => 'desc' })

Clicking the resulting link will toggle the sort directions of both
name and kind, sorting the name field by default ascending, and
the kind field descending.

If you wanted to specify a label, you would add it after the array or sort fields:

sort_link(:kind, [:kind, :name], "Property Kind", :default_order => 'asc')

caleb added 2 commits October 2, 2014 00:54
This patch allows users to sort on multiple fields with the sort_link
helper.

To specify sorting on multiple fields:

  sort_link(:kind, [:kind, 'name asc'])

This will create a sort link that sorts first by kind, and then by
name. The first `:kind` parameter ensures that the link generated
shows the sort status of the `kind` field.

When you specify a sort direction in the sort fields array, the
direction is locked to that direction. In the above example, clicking
the resulting link would toggle sorting of the kind field, but the name
field would always sort ascending.

Also added was the ability to specify multiple default_order fields
with a hash:

  sort_link(:kind, [:kind, :name],
            :default_order => { :name => 'asc', :kind => 'desc' })

Clicking the resulting link will toggle the sort directions of both
`name` and `kind`, sorting the `name` field by default ascending, and
the `kind` field descending.
This spec ensures that the user can provide a label when specifying
multiple sort fields
@jonatack
Copy link
Contributor

jonatack commented Oct 2, 2014

@caleb This is a great addition. I've been working around this limitation in my applications for so long I hadn't considered adding this. We just need to get the tests green for Rails 3.1 and 3.2, which ought to be quick. Let's merge this in so people can start trying it out and fix the Rails 3.1/3.2 compatibility ASAP.

jonatack added a commit that referenced this pull request Oct 2, 2014
Add multiple sort field support in sort_link [skip ci]
@jonatack jonatack merged commit 75bf52b into activerecord-hackery:master Oct 2, 2014
@jonatack
Copy link
Contributor

jonatack commented Oct 2, 2014

Thanks 💛

jonatack added a commit that referenced this pull request Oct 2, 2014
@jonatack jonatack mentioned this pull request Oct 2, 2014
@caleb
Copy link
Contributor Author

caleb commented Oct 2, 2014

Thanks for fixing rails 3! I was going to work on that tonight.

@jonatack
Copy link
Contributor

jonatack commented Oct 2, 2014

Would you like to add a PR to update the sort_link docs in the README?

(Don't forget to use [skip ci] in your PR for documentation-only changes).

@caleb
Copy link
Contributor Author

caleb commented Oct 2, 2014

I will do that tonight.

On Oct 2, 2014, at 11:48 AM, Jon Atack notifications@github.com wrote:

Would you like to add a PR to update the sort_link docs in the README?

(Don't forget to use [skip ci] in your PR for documentation-only changes).


Reply to this email directly or view it on GitHub.

@jonatack
Copy link
Contributor

jonatack commented Oct 6, 2014

I've found what seems to be a bug introduced by this PR. If neither a default_order nor a custom label is specified, sort_link no longer picks up the translation from the I18n .yml files and no label is displayed at all.

@caleb
Copy link
Contributor Author

caleb commented Oct 6, 2014

Okay. I can look into this today or tomorrow.

@jonatack
Copy link
Contributor

jonatack commented Oct 6, 2014

Great, thank you.

@caleb
Copy link
Contributor Author

caleb commented Oct 6, 2014

Could you give me some example code showing the failing case? I will make a test case out of it.

@jonatack
Copy link
Contributor

jonatack commented Oct 6, 2014

The following displays 'Website' in the label (and works fine):

<%= sort_link(@q, :website, 'Website') %>
<%= sort_link(@q, :website, [:posts_count, :name], 'Website') %>
<%= sort_link(@q, :website, [:posts_count, :name], default_order: :desc) %>

But the following shows nothing:

<%= sort_link(@q, :website) %>
<%= sort_link(@q, :website, [:posts_count, :name]) %>

@jonatack
Copy link
Contributor

jonatack commented Oct 6, 2014

Using Rails master and Ruby 2.1.3 p242. But all worked fine before the PR, so I don't think the Rails version is changing anything here.

@caleb
Copy link
Contributor Author

caleb commented Oct 6, 2014

Thank you. I thought there was a test that covered the broken case, but apparently not. I will dive into it tonight.

@jonatack
Copy link
Contributor

jonatack commented Oct 6, 2014

That would be great. We could definitely use better test coverage.

jonatack referenced this pull request Oct 26, 2014
Not passing an options hash into sort_link caused it to fail
to look up the link title.

This is because `!args.first.try(:is_a?, Hash)` would return true
if the length of args was 0.

I've simplified the logic to just test if the next parameter in args
is a String (rather than test if it's not a Hash).
@tylercollier
Copy link

@caleb Awesome feature!

I have one request for you, although really for the maintainers (@jonatack). After reading the documentation in the README and trying to get this to work for an hour, it was only then that I found this github issue and realized this might not be a released feature. I think it would be awesome for apparently-no-so-veteran people like me to put a note in the README that says something like the following, which I've copied from jquery-cookie:

If you're viewing this at https://github.com/carhartl/jquery-cookie, you're reading the documentation for the master branch. View documentation for the latest release (1.4.1).

@jonatack
Copy link
Contributor

@tylercollier thanks for the feedback. If you look at the CHANGELOG, this feature was merged into a general release (Ransack 1.5.0) on October 26, 2014: 2 releases ago, and soon to be 3. What version of Ransack is not working for you?

@tylercollier
Copy link

My fault! I was using version 1.4.1. Apparently it was included from active admin, which I added to my project on 2014-09-30, a week after 1.4.1 was released.

So in this case, my suggestion wouldn't have helped me, since I was the one who was on an old version. Still, I think it's a good idea that many READMEs are including for git(hub) repos these days.

Regardless, thanks for the fast response, and keep up the good work!

@jonatack
Copy link
Contributor

Sure, glad you got it sorted.

@jonatack
Copy link
Contributor

@tylercollier I added your suggestion to the README. Thanks for the idea!

@tylercollier
Copy link

Thank you! I'm sure it'll help folks.

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

Successfully merging this pull request may close these issues.

3 participants