josefarias / hotwire_combobox

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

Possible prefilled display value issues #123

Closed thanosbellos closed 4 months ago

thanosbellos commented 4 months ago

Hello again.

I have been using the gem for a while and there have been 2 cases where prefilled display values are not displayed.

  1. Trying to edit a resource's free text attribute using async load.
  2. Trying to edit a resource with an association to another grouped resource.

I will try to explain the issues

  1. This issue arises when we don't have an association but we load async from a remote source and also allow new values to be inserted. A use case scenario: We have a list of template names that the user may choose from, but can also insert a new one. The template names may thousands of records, thus we want to load them asynchronously.

https://github.com/josefarias/hotwire_combobox/blob/002eb00c851fc179f51e5f1f7125101c4040562f/app/presenters/hotwire_combobox/component.rb#L240-L248

It seems logical that there won't be any options to search from on line 246 as we will load them asynchronously. Based on the documentation that may be a little bit difficult to handle. The most naive solution would be to just return the hidden_field_value but I think that this would only handle cases where the option display value and the value are identical).

  1. the second case scenario is the following:

We have a form for a resource with an association to some another resource that can be grouped (i.e user wants to select a favorite movie, and we display a list of movies grouped by their genre).

When we first choose a movie everything works greatly. When we try to visit the page to edit his choice, we get "undefined method `value' for an instance of HotwireCombobox::Listbox::Group". the search logic in the following lines https://github.com/josefarias/hotwire_combobox/blob/002eb00c851fc179f51e5f1f7125101c4040562f/app/presenters/hotwire_combobox/component.rb#L240-L248

will fail because the options variable is an array of HotwireCombobox::Listbox::Group instances, that do not respond to the value method.

I tried to iterate through each group options and find the selected one, but HotwireCombobox::Listbox::Group#options is private.

I will happily create a repo that replicates both issues, if you need more information :)

josefarias commented 4 months ago

Thanks for a great report @thanosbellos! I think I understand the second use case well enough but not the first. If you don't mind, that repo you mentioned reproducing both scenarios would be fantastically helpful in solving these.

thanosbellos commented 4 months ago

Here is a dummy app with both of those scenarios. I have some description of both scenarios in the readme file.

Some quick info for the first scenario: We have a resource (i.e user) and we want to be able to:

  1. asynchronously search and select from a pool of already inserted values for an attribute of the resource(i.e user name)
  2. add a new option if there isn't one the pool of options by typing some text.

when we try to edit the attribute again, the display value of the combobox starts empty.

josefarias commented 4 months ago

@thanosbellos I really, really appreciate the time and effort you put into sharing a reproduction — with instructions and seeds and everything. It was super, super useful. Thank you.

I now understand both use cases and have started working on a fix for number 2 here: https://github.com/josefarias/hotwire_combobox/pull/133

Will wrap that up when I get some more time and then tackle number 1 in a separate PR.

josefarias commented 4 months ago

Both PRs have been merged. This should all be fixed on main. Let me know if you run into trouble. Thanks!

thanosbellos commented 4 months ago

@josefarias thanks for the incredibly quick fixes. I really appreciate all of your efforts into this library.

The 2nd case is indeed fixed.

I still have a minor issue with the first use case.

Keep in mind that in the first scenario there isn't an association to select from. the pool of the possible values may be an arbitrary array of strings or as in my case, the distinct values of a column (possible movie names, project names, user names and other similar examples).

It seems that the data value is inserted in the hidden field as a value whenever I select an already defined name. it's not limited to editing the resource, it seems to be the case even when I even create a new resource and choose a value for the attribute. I am attaching a clip for the issue that may have been already present and I must have missed it. I will create a new issue if you want cause it may be unrelated to this one.

https://github.com/josefarias/hotwire_combobox/assets/425283/3a495ca4-9c73-4165-98b9-7b93b9e6f20d

josefarias commented 4 months ago

I'll take a look!

josefarias commented 4 months ago

Hey @thanosbellos I think this is working as intended. It looks like you're having two issues, if I understood correctly:

  1. When selecting an existing name from the combobox options — instead of persisting the name, a number is persisted. In this case 3.
  2. The values are still not pre-filled for you

For number 1, what's happening is that you're providing an ActiveRecord::Relation as the options. In this case, it's the user records.

The combobox is designed to call id when you provide records as the options. So the 3 you're seeing there is the id of the user called thanos, which you selected when creating a new user. Now you have two users: thanos and 3. When you go to edit the second user and type foobar — the name 3 is updated to foobar because the combobox accepts freetext input. Now your two users are called thanos and foobar. Finally, when you edit foobar and choose thanos as their new name, you get 3 again because that's the id for the user named thanos.

I think this is what you'd want to do instead:

diff --git a/app/views/users/_user.turbo_stream.erb b/app/views/users/_user.turbo_stream.erb
index f52a5d1..b07c35a 100644
--- a/app/views/users/_user.turbo_stream.erb
+++ b/app/views/users/_user.turbo_stream.erb
@@ -1,3 +1,3 @@
 <div class="relative text-gray-900 cursor-default select-none">
-  <span class="block truncate"><%= user.name %></span>
+  <span class="block truncate"><%= combobox_display %></span>
 </div>
diff --git a/app/views/users/autocomplete.turbo_stream.erb b/app/views/users/autocomplete.turbo_stream.erb
index 8d66d67..1416650 100644
--- a/app/views/users/autocomplete.turbo_stream.erb
+++ b/app/views/users/autocomplete.turbo_stream.erb
@@ -1 +1 @@
-<%= async_combobox_options @users || [], render_in: {partial: "users/user"}, next_page: @pagy.next %>
+<%= async_combobox_options @users.pluck(:name), render_in: {partial: "users/user"}, next_page: @pagy.next %>

Now you'd be passing the names as the options. And when passing an array of strings to the combobox, both the display and value are the literal string for each option.

For number 2, I think you just need to point your gem to the latest release 0.2.1 — you're most likely still running 0.2.0.

Closing for now but let me know if things still seem broken. Thanks!

thanosbellos commented 4 months ago

Aah sorry, I had tested the new gem version locally and forgot to push on the dummy app. Yes the values were indeed pre filled.

Wow great catch about the second one with the ids. It works perfectly fine after testing your suggestion.

Again many thanks for all of your feedback.