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

Simple form integration #1487

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

abinoam
Copy link

@abinoam abinoam commented Apr 8, 2024

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 added 9 commits April 7, 2024 21:17
* It inherits from normal Ransack::Helpers::FormBuilder
* It composes with an instance of SimpleForm:Builder,
  so it may delegate methods like SimpleForm "input"

Next:
- Extract "label_text"
- Resolve simple_form dependency
This allows SimpleForm to use the correct label like
  "name_cont" -> "Name contains"
It will not be "required" by default.
So it doesn't increases gem's size.
It checks if SimpleForm#input method is callable.
And checks for correct wrapping of a normal text field.
@abinoam abinoam force-pushed the simple_form_integration branch from 7c8985a to 1886b10 Compare April 9, 2024 03:32
abinoam added 3 commits April 13, 2024 22:01
It should generate boolean fields whe the predicate is boolean.
  e.g.: _blank, _present, _true, _not_null

It should respect the type of the ActiveRecord attribute otherwise.
  e.g.: Generate the proper date field for date attributes.
Integrating with existing methods,
even those not designated as 'public API',
to minimize code duplication and maintain compatibility with future changes.
@abinoam abinoam changed the title [WIP] Simple form integration Simple form integration Apr 14, 2024
@abinoam
Copy link
Author

abinoam commented Apr 14, 2024

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 abinoam marked this pull request as ready for review April 14, 2024 02:33
@scarroll32
Copy link
Member

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

@abinoam
Copy link
Author

abinoam commented Apr 24, 2024

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:

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! 👏

@scarroll32
Copy link
Member

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
Copy link
Member

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

@abinoam
Copy link
Author

abinoam commented Apr 29, 2024

@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! 😄🚀

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