Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add "order nulls last" option #242

Closed
wants to merge 30 commits into from

Conversation

natebird
Copy link
Contributor

@natebird natebird commented Jul 7, 2017

  • Allows for nulls to be ordered last using a global configuration option using config.nulls_last = true in the ajax_datatables_rails.rb file.
  • Allows for nulls to be ordered last on individual columns by adding nulls_last: true to the view_columns method in the datatable class.

@natebird natebird mentioned this pull request Jul 7, 2017
@natebird
Copy link
Contributor Author

natebird commented Jul 7, 2017

I'm open to also creating a per column option. But I'm unsure where that would go and how to pass it to the SimpleOrder class. Any ideas would be helpful. Thanks!

@ajahongir
Copy link
Collaborator

well, you have to add method a public into column.rb

def nulls_last
  @view_column.fetch(:nulls_last, true)
end

and then you can access from SimpleOrder as column.nulls_last

@natebird
Copy link
Contributor Author

@ajahongir I'm running into an issue in the test where I can't call column because the datatable doesn't have the column_by method. Can you help me understand why that isn't available? It seems to be only a testing issue.

@ajahongir
Copy link
Collaborator

where is your test falling?

@natebird
Copy link
Contributor Author

natebird commented Sep 14, 2017

It's failing in the simple_order_spec. But it traces back to simple_order.rb where I'm calling column.nulls_last https://github.com/jbox-web/ajax-datatables-rails/pull/242/files#diff-4f1e2d7f5a934d960a24dbd4ab6929e4R25. column blows up because it can't return a value because the column_by method doesn't exist for the @datatable.

@@ -20,6 +20,14 @@ def column
def direction
DIRECTIONS.find { |dir| dir == @options[:dir].upcase } || 'ASC'
end

def sort_nulls_last(sort_column)
if column.nulls_last == true || AjaxDatatablesRails.config.nulls_last == true
Copy link
Collaborator

Choose a reason for hiding this comment

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

sort_column.nulls_last ?

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 sort_column is just a string so that doesn't work. I need access to the column options.

@@ -1,13 +1,37 @@
require 'spec_helper'

describe AjaxDatatablesRails::Datatable::SimpleOrder do

let(:datatable) { ReallyComplexDatatable.new(double('view', params: sample_params)) }
let(:sorted_datatable) { DatatableOrderNullsLast.new(double('view', params: sample_params)) }
Copy link
Collaborator

Choose a reason for hiding this comment

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

it seams here you datatable instance returning nil

@natebird
Copy link
Contributor Author

So, I got things worked out. But the build is failing on the Oracle tests. I don't think it is related to my changes. What do I need to do to get them passing?

@natebird
Copy link
Contributor Author

Also, does this need readme documentation? I'm happy to add that to this PR if you want it.

@natebird
Copy link
Contributor Author

natebird commented Oct 9, 2017

Any ideas on how to get the Oracle tests passing?

@natebird
Copy link
Contributor Author

What can I do to get this Oracle test passing?

@natebird natebird changed the title Order nulls last Add "order nulls last" option Jan 23, 2018
@n-rodriguez
Copy link
Member

@natebird the Oracle tests now pass, can you please rebase?

@natebird
Copy link
Contributor Author

Yep. I’ll do it later tonight. Thanks!

@n-rodriguez
Copy link
Member

be careful there's a lot of changes :/

@natebird
Copy link
Contributor Author

Closing in favor of the cleaner, #279 PR. I couldn't do a clean rebase so I just recreated the branch. :-)

@natebird natebird closed this Feb 20, 2018
@natebird natebird deleted the order_nulls_last branch January 16, 2019 18:42
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants