add functionality to hide all search indicator arrows via config #577
Conversation
Changed the description of the 'false' case spec to be more accurate.
fixed formatting of code portion of description
| @@ -198,7 +198,7 @@ def default_sort_order(attr_name) | |||
| end | |||
|
|
|||
| def order_indicator | |||
| if @hide_indicator || no_sort_direction_specified? | |||
| if @hide_indicator || no_sort_direction_specified? || Ransack.options[:remove_search_order_indicators] | |||
There was a problem hiding this comment.
Perhaps better to do this in SortLink#initialize when @hide_indicator is set up?
|
Thanks! Looks pretty good. I made a few comments in the code. |
…ved config option name and readme
|
You bet! I'm glad you like it! |
|
|
||
| ```ruby | ||
| Ransack.configure do |c| | ||
| c.remove_search_order_indicators = true |
There was a problem hiding this comment.
Perhaps should be updated to c.hide_sort_order_indicators = true?
There was a problem hiding this comment.
Thanks for the better formatting!
| controller: 'people' | ||
| ) | ||
| } | ||
| it { should match /Full Name/ } |
There was a problem hiding this comment.
- Does this test ever fail? Perhaps it
should notmatch▼and also not match the other arrow to be safe, WDYT? - Could you please use RSpec
expectinstead ofshouldsyntax? We've been migrating all the specs in that direction.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
To make the test work properly, I'd suggest:
it { should_not match /▼|▲/ }It should pass when hide_sort_order_indicators = true and fail when hide_sort_order_indicators = false.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
If all this is confusing, don't worry about it and let me know, and I'll just commit it and clean it up.
There was a problem hiding this comment.
Good catch, the spec for hide_indicator has the same problem.
There was a problem hiding this comment.
You're right, good catch, that test wasn't working 👍
…e regex in the hide_indicator spec and hide_sort_order_indicator spec; corrected option name in hide_sort_orders setter in configuration.rb
…t_link specs and corrected their description
|
I think I got everything. Thanks for your patience! This is my first contribution to a gem and I really appreciate the feedback and insights! |
|
All good! Here is what remains: To keep the git history clean and clear, we need to merge the commits into one, but only the ones relevant to the new feature, and update the change log. The change that fixes the other spec, and the change to the We have two options. You can do it, but it might be a little complicated if you haven't squashed/cherry-picked/rebased commits before (I can help you if needed). Or I can do it. The result will be the same. Let me know! |
|
P.S. It's great that working together, the bug in the other spec was found and fixed. Two pairs of eyes are better than one. |
|
If you don't mind, I'd rather leave that stuff up to you. Thanks again! |
|
I don't mind. Thanks for the contributions! |
|
P.S. It looks like you have different user names/emails between your github account and your local |
|
Once your |
config/initializer/ransack.rb
Ransack.configure do |c|
c.remove_search_order_indicators = true
end
This is basically like passing 'hide_indicator: true' to all of your search indicators..without passing it to all of your search indicators.