ransack icon indicating copy to clipboard operation
ransack copied to clipboard

Simple form integration

Open abinoam opened this issue 10 months ago • 6 comments

EDIT

This PR will be turned into a separate gem. I'll let it open for discussion until we release them.


Related to #1160

This PR introduces the Ransack::Helpers::SimpleFormBuilder, which extends Ransack::Helpers::FormBuilder and integrates SimpleForm::FormBuilder. This approach avoids the need for monkey patching, using environment variables, or potential code collisions by:

  • Establishing a distinct Ransack::Helpers::SimpleFormBuilder to facilitate integration with SimpleForm.
  • Allowing concurrent use of Ransack, SimpleForm, and the combined form builders within the same application through the new search_simple_form_for method.
  • Providing has_attribute? and type_for_attribute methods to enable SimpleForm to accurately infer field types.
  • Ensuring that boolean predicates such as _blank or _not_null are correctly handled by generating appropriate boolean fields.
  • Striking out previous limitations regarding the handling of non-text fields.

Additional contributions include:

  • Adding specific tests for this feature.
  • Updating the documentation to reflect these changes.

After this PR one can use SimpleForm like:

<%= search_simple_form_for @q do |f| %>
  <%= f.input :name_cont %>
  <%= f.input :employee_name_present %>
  <%= f.input :created_at_gteq %>
  <%= f.button :submit %>
<% end %>

I am currently conducting further tests and welcome any feedback on this PR. I would appreciate any contributions to refine this feature and am looking forward to potentially merging it.

abinoam avatar Apr 08 '24 17:04 abinoam

Here some screenshots of testing the feature in my app.

  • employee_name_present is correctly identified as a boolean predicate and a check box is rendered.
  • creation_date_gteq is correctly identified as a datetime attribute and the selects are rendered.
  • Compound conditions like name_or_destination_name_cont are handled correctly.
  • The labels are correctly translated to the default locale (pt-BR)
  • The Bootstrap 5 wrapper divs are put in place

Captura de tela de 2024-04-13 20-54-58

Screenshot 2024-04-13 at 20-53-30 Gomedical

abinoam avatar Apr 14 '24 02:04 abinoam

@abinoam would you consider publishing this as a separate gem?

scarroll32 avatar Apr 24 '24 20:04 scarroll32

Hi @scarroll32,

I completely understand your perspective, and ideally, making a separate gem would be a good approach. This PR is submitted directly to Ransack mainly because there is already a preliminary, albeit non-functional, integration in place.

Ransack's form builder methods.

The current structure of Ransack’s code doesn’t clearly define extension points for the functionality this PR introduces. I found myself having to utilize send to access private methods, which isn’t ideal for maintenance or clarity (from a external gem perspective).

Given these constraints, creating an external gem without a stable public API to hook into would be likely prone to break with future updates. By integrating these changes directly within Ransack, we can ensure any incompatibilities are immediately flagged by failing tests, thus maintaining stability.

However, if the guidance is to minimize coupling with code that is not intrinsic to Ransack as much as possible, I would be happy to convert this PR into a separate gem. In this case, I would also suggest considering the removal of the current integration code, since it has caused frustrations due to not working properly (I could do a PR to remove it).

I also would ask you if you would accept a PR that extract some methods into public apis in Ransack so I could hook into them from the external gem? These would not alter in any form the current behavior of Ransack.

It's important to note that there's already gems that do that. I tested and they didn't work as I expected:

  • https://github.com/kaspernj/simple_form_ransack
  • https://github.com/iquest/ransack_simple_form

One of the thing that is noticeable in these gems it's that they rewrite some Ransack code. So, when Ransack changes, it stops working. I don't plan to incur in the same problem.

Let me know your final verdict and I will start to work on it.

Thanks for your time in maintaining Ransack! 👏

abinoam avatar Apr 24 '24 21:04 abinoam

However, if the guidance is to minimize coupling with code that is not intrinsic to Ransack as much as possible, I would be happy to convert this PR into a separate gem. In this case, I would also suggest considering the removal of the current integration code, since it has caused frustrations due to not working properly (I could do a PR to remove it).

@abinoam thank you for your PR and these comments. It's great to have someone interested in contributing to Ransack... I am hesitant to add code for a library that is used by some users only. There was a similar discussion recently about full text search.

I also would ask you if you would accept a PR that extract some methods into public apis in Ransack so I could hook into them from the external gem? These would not alter in any form the current behavior of Ransack.

This sounds fine, I'd really appreciate any refactoring that you can do on this! We can also add a link to the new gem in the Ransack docs.

scarroll32 avatar Apr 26 '24 20:04 scarroll32

See also https://activerecord-hackery.github.io/ransack/going-further/other-notes/#using-simpleform

scarroll32 avatar Apr 27 '24 09:04 scarroll32

@abinoam thank you for your PR and these comments. It's great to have someone interested in contributing to Ransack... I am hesitant to add code for a library that is used by some users only. There was a similar discussion recently about full text search.

I completely understand you.

I also would ask you if you would accept a PR that extract some methods into public apis in Ransack so I could hook into them from the external gem? These would not alter in any form the current behavior of Ransack.

This sounds fine, I'd really appreciate any refactoring that you can do on this! We can also add a link to the new gem in the Ransack docs.

Nice. I'll get working on that gem soon— probably not this week, but it's on my to-do list! I definitely need it for a side project of mine, so it will happen! 😄🚀

abinoam avatar Apr 29 '24 14:04 abinoam