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 customizable indicator arrows #726

Merged

Conversation

garettarrowood
Copy link
Contributor

This addition allows users to pass in their own up and down arrows. The default arrows remain the same. The user can pass in just one arrow, or both arrows.

Functionality is hand-tested with a codebase in production. Please let me know if you'd like any edits.

@garettarrowood
Copy link
Contributor Author

Sorry about the extra CI builds. Just realized it's been running a few times as I appended the documentation. Tried to log in and stop the last one to no avail. I'll be sure to include the [skip ci] option should a need to alter any documentation again.

Copy link
Contributor

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request! See the few minor comments above but globally looks good. Well done.


### Added

* Add config option to customize up and down arrows used for direction indicators in the FormHelper. PR [#726](https://github.com/activerecord-hackery/ransack/pull/726)
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 please wrap at 80 characters in the source file?

@@ -213,8 +213,18 @@ 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 file like `config/initializers/ransack.rb`:
These indicator arrows may also be customized by setting them in an initializer file 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.

Same here :)

end
```

Alternatively, all sort links may be displayed without the order indicator arrows:
Copy link
Contributor

Choose a reason for hiding this comment

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

and here :)

# like `config/initializers/ransack.rb` as follows:
#
# Ransack.configure do |config|
# # Set the up_arrow as an icon, set the down arrow as unicode
Copy link
Contributor

@jonatack jonatack Sep 28, 2016

Choose a reason for hiding this comment

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

For line 86, I'd suggest # # Set the up arrow as an icon, and the down arrow as unicode.

(Note: "up arrow" written as text instead of in snakecase)

end

def down_arrow
'&#9650;'.freeze
Ransack.options[:down_arrow]
end
Copy link
Contributor

@jonatack jonatack Sep 28, 2016

Choose a reason for hiding this comment

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

I originally extracted these 2 public arrow methods out from the private method #order_indicator to allow developers to set custom arrows by overloading them.

Now that developers can do it with a config setting, I'm not sure there is any reason to keep them as separate public methods. Performance-wise it would be better to inline them.

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, except if anyone is actually overloading these methods now, their apps will break. Never mind 😄

@jonatack jonatack merged commit 6fcb168 into activerecord-hackery:master Sep 29, 2016
@jonatack
Copy link
Contributor

@garettarrowood The changes were so minor that let's just merge this in and I'll do a mini-cleanup. Thanks again for the clean PR with tests.

@garettarrowood
Copy link
Contributor Author

@jonatack Cool! Thank you for the merge and cleaning up those comments/docs.

jonatack added a commit that referenced this pull request Sep 29, 2016
@jonatack
Copy link
Contributor

@garettarrowood Thanks again for your contribution!

jonatack added a commit that referenced this pull request Sep 29, 2016
jonatack added a commit that referenced this pull request Sep 29, 2016
jonatack added a commit that referenced this pull request Sep 29, 2016
TODO: Avoid cloning and restoring Ransack.options in each test.
jonatack added a commit that referenced this pull request Sep 29, 2016
@garettarrowood
Copy link
Contributor Author

Great refactoring and additions to the tests. I enjoyed reviewing your edits.

jonatack added a commit that referenced this pull request Sep 30, 2016
@jonatack
Copy link
Contributor

Thanks @garettarrowood.

I'm a bit bothered by all the tests duplicated for the two adapters. Code bloat. Probably should be fixed.

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.

2 participants