josefarias / hotwire_combobox

An accessible autocomplete for Ruby on Rails.
https://hotwirecombobox.com
MIT License
436 stars 26 forks source link

Disable "`Model` does not respond to `#to_combobox_display`." error #124

Closed Paul-Bob closed 4 months ago

Paul-Bob commented 4 months ago

Hello there, cool project! Cheer for all the contributors!

I'm using the render_in option passing a partial and still get the error "Model does not respond to #to_combobox_display. "

The name is computed on the partial never using the to_combobox_display model method.

Is there a way to disable this error since the method result is never actually used on my use case?

Thank you!

Paul-Bob commented 4 months ago

Just had a look to the source code and found the answer to my own question, passing display: false on the async_combobox_options disables to_combobox_display call.

josefarias commented 4 months ago

Hey @Paul-Bob! The display value is also used to perform the autocomplete and filtering, it's not just for rendering the option contents. Are those working for you?

If you override the display value, you'll have to pass filterable_as and autocompletable_as to get those functionalities. Unless I'm missing something 🤔.

My suggestion would be to implement the method. Passing those options shouldn't be necessary in the overwhelming majority of cases.

Paul-Bob commented 4 months ago

Actually I misunderstood the display option, it will use the given method to display the selected option. Fixed using:

      render turbo_stream: helpers.async_combobox_options(
        @options,
        next_page: @pagy.next,
        display: Proc.new { |record| @attachment_resource.new(record: record).record_title }
      )
Paul-Bob commented 4 months ago

My suggestion would be to implement the method.

I have no access over the parent app models code since I'm experimenting it as an Avo feature. I would need to implement this method dynamically on each model, it's doable but maybe the parent app is already using the to_combobox_display method itself for other scenario, makes sense?

josefarias commented 4 months ago

I'm lacking enough context to make a worthwhile suggestion. But to_combobox_display is a fairly specific name for a method. I'd say it'd be unusual that the implementing user is already using it for something else. I'd wait until someone actually reports that they have a naming conflict and in that case they can pass display: :some_other_method to the options, instead of display: false.

Paul-Bob commented 4 months ago

I'm lacking enough context to make a worthwhile suggestion.

On Avo we offer a select prefilled with all the possible options, for example when attaching records on a association:

image

This approach sometimes can take long or break since there can be a lot of possible options specially on a production app.

In order to fix it we're considering hotwire_combobox so the options are fetched and paginated async.

The old options for select was geting coputed like this:

@options = query.all.map do |record|
  [@attachment_resource.new(record: record).record_title, record.id]
end

To keep the same old logic for fetching the names I ended up with:

@pagy, @options = pagy query.all
render turbo_stream: helpers.async_combobox_options(
  @options,
  next_page: @pagy.next,
  display: Proc.new { |record| @attachment_resource.new(record: record).record_title }
)

But to_combobox_display is a fairly specific name for a method. I'd say it'd be unusual that the implementing user is already using it for something else.

Totally agree, if someone is using to_combobox_display method on a record It's definitively for hotwire_combobox and we don't want it to be overridden when installing Avo. What I mean is that the project can be already using hotwire_combobox and the to_combobox_display on other parts of the application that are not Avo related and we would like to keep the old naming logic without accessing the model method.

The solution that I shared above seems to be working

josefarias commented 4 months ago

Ah I think I understand the situation now.

Yes! That solution from above should work perfectly. Let me know if not. Kudos for finding that, that's pretty advanced usage that's not documented at all.

Paul-Bob commented 4 months ago

Thank you!