-
-
Notifications
You must be signed in to change notification settings - Fork 788
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
base: main
Are you sure you want to change the base?
Simple form integration #1487
Conversation
Next: - Implement the builder
* 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.
7c8985a
to
1886b10
Compare
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 would you consider publishing this as a separate gem? |
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! 👏 |
@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.
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. |
I completely understand you.
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! 😄🚀 |
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 extendsRansack::Helpers::FormBuilder
and integratesSimpleForm::FormBuilder
. This approach avoids the need for monkey patching, using environment variables, or potential code collisions by:Ransack::Helpers::SimpleFormBuilder
to facilitate integration with SimpleForm.search_simple_form_for
method.has_attribute?
andtype_for_attribute
methods to enable SimpleForm to accurately infer field types.Additional contributions include:
After this PR one can use SimpleForm like:
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.