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 functionality to hide all search indicator arrows via config #577

9 changes: 9 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,15 @@ The sort link may be displayed without the order indicator arrow by passing
<%= sort_link(@q, :name, hide_indicator: true) %>
```

Alternatively, all sort links may be displayed without the order indicator arrow
by adding this to an initializer like `config/initializers/ransack.rb`:

Copy link
Contributor

Choose a reason for hiding this comment

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

Please use markdown ruby code format block for the config code example (3 backticks + 'ruby' ... code ... 3 backticks).

  code

```ruby
Ransack.configure do |c|
c.remove_search_order_indicators = true
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps should be updated to c.hide_sort_order_indicators = true?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the better formatting!

end
```

### Advanced Mode

"Advanced" searches (ab)use Rails' nested attributes functionality in order to
Expand Down
7 changes: 6 additions & 1 deletion lib/ransack/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ module Configuration
self.predicates = {}
self.options = {
:search_key => :q,
:ignore_unknown_conditions => true
:ignore_unknown_conditions => true,
:hide_sort_order_indicators => false
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps call it :hide_sort_order_indicators, WDYT?


def configure
Expand Down Expand Up @@ -75,5 +76,9 @@ def arel_predicate_with_suffix(arel_predicate, suffix)
end
end

def hide_sort_order_indicators=(boolean)
self.options[:remove_search_order_indicators] = boolean
Copy link
Contributor

Choose a reason for hiding this comment

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

This was not updated to the new name yet, so the code was not working.

end

end
end
8 changes: 6 additions & 2 deletions lib/ransack/helpers/form_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ def initialize(search, attribute, args, params)
@current_dir = existing_sort_direction
@label_text = extract_label_and_mutate_args!(args)
@options = extract_options_and_mutate_args!(args)
@hide_indicator = @options.delete :hide_indicator
@hide_indicator = indicator?
Copy link
Contributor

Choose a reason for hiding this comment

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

The indicator? method is a bit misnamed, it is really checking if Ransack should hide_indicator? and the logic in that method appears to be backward.

It might be better to inline this method in the constructor with this, WDYT?

@hide_indicator = @options.delete(:hide_indicator) || Ransack.options[:hide_sort_order_indicators]

and remove the indicator? method.

@default_order = @options.delete :default_order
end

Expand Down Expand Up @@ -197,8 +197,12 @@ def default_sort_order(attr_name)
end
end

def indicator?
@options.delete :hide_indicator unless Ransack.options[:hide_sort_order_indicators]
end

def order_indicator
if @hide_indicator || no_sort_direction_specified?
if @hide_indicator || no_sort_direction_specified?
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove the extra whitespace at the end of line 205?

We could shorten this to:

def order_indicator
  return if @hide_indicator || no_sort_direction_specified?
  direction_arrow
end

nil
else
direction_arrow
Expand Down
28 changes: 28 additions & 0 deletions spec/ransack/helpers/form_helper_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -399,6 +399,34 @@ module Helpers
it { should match /example_name_eq/ }
end

describe '#search_form_for with config set to remove sort order indicators' do
before do
Ransack.configure { |c| c.hide_sort_order_indicators = true }
end
subject { @controller.view_context
.sort_link(
[:main_app, Person.search(sorts: ['name desc'])],
:name,
controller: 'people'
)
}
it { should match /Full Name/ }
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Does this test ever fail? Perhaps it should not match &#9660; and also not match the other arrow to be safe, WDYT?
  2. Could you please use RSpec expect instead of should syntax? We've been migrating all the specs in that direction.

Choose a reason for hiding this comment

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

I think that if it is matching /Full Name/ it is automatically not matching either arrow. I literally just copied/pasted that spec from the 'hide_indicator => true' spec because I figured the behavior is the same, but the implementation is different.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just tried that test with the inverse condition hide_sort_order_indicators = false and it passes instead of failing, because it will always match to the attribute name which is present either way.

Copy link
Contributor

Choose a reason for hiding this comment

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

To make the test work properly, I'd suggest:

it { should_not match /&#9660;|&#9650;/ }

It should pass when hide_sort_order_indicators = true and fail when hide_sort_order_indicators = false.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK for leaving the tests with RSpec should syntax since the rest of that file is that way. It may be better to move these 2 tests up to line 356 at the end of the sort_link tests where the other hide indicator specs are, and describe them like the other tests too, i.e. 'sort_link with...' instead of 'search_form_for with...' since they relate to the sort_link method.

Copy link
Contributor

Choose a reason for hiding this comment

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

If all this is confusing, don't worry about it and let me know, and I'll just commit it and clean it up.

Choose a reason for hiding this comment

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

Good catch, the spec for hide_indicator has the same problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right, good catch, that test wasn't working 👍

end

describe '#search_form_for with config set to remove sort order indicators' do
before do
Ransack.configure { |c| c.hide_sort_order_indicators = false }
end
subject { @controller.view_context
.sort_link(
[:main_app, Person.search(sorts: ['name desc'])],
:name,
controller: 'people'
)
}
it { should match /Full Name&nbsp;&#9660;/ }
end

end
end
end