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

open listbox only if it contains options #179

Closed
wants to merge 1 commit into from

Conversation

robinboening
Copy link

In case the listbox doesn't have any options it doesn't need to be shown.
See this discussion for reference: #178

@robinboening
Copy link
Author

@josefarias I wasn't comfortable touching the tests as I am not familiar enough with the setup and all the helpers. Would you mind giving me a hand if you think this should be tested?

josefarias
josefarias previously approved these changes Jul 2, 2024
Copy link
Owner

@josefarias josefarias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @robinboening!

I think we can merge this as-is for now. I'm trying to keep the tests mostly functional, and this is arguably presentational.

Might come back and test this later if necessary.

@josefarias josefarias dismissed their stale review July 2, 2024 22:54

Oh, wasn't expecting any test failures. We should check those out before merging.

@josefarias
Copy link
Owner

Ah I see what's happening. Prefilled async comboboxes won't work with this change because we need to show the turbo-frame in order to load it lazily.

I'm not able to reproduce this on main anymore though. Am I doing something wrong? I tried with both async and sync comboboxes by making the following changes:

diff --git a/test/dummy/app/controllers/movies_controller.rb b/test/dummy/app/controllers/movies_controller.rb
index 4dd8cdf..924dd38 100644
--- a/test/dummy/app/controllers/movies_controller.rb
+++ b/test/dummy/app/controllers/movies_controller.rb
@@ -15,7 +15,7 @@ class MoviesController < ApplicationController
 
   private
     def set_page
-      movies = params[:full_search] ? Movie.full_search(params[:q]) : Movie.search(params[:q])
+      movies = Movie.none
       set_page_and_extract_portion_from movies.alphabetically, per_page: 5
     end
 end
diff --git a/test/dummy/app/views/comboboxes/plain.html.erb b/test/dummy/app/views/comboboxes/plain.html.erb
index 8a7f5fe..14cce43 100644
--- a/test/dummy/app/views/comboboxes/plain.html.erb
+++ b/test/dummy/app/views/comboboxes/plain.html.erb
@@ -1,3 +1,3 @@
-<%= combobox_tag "state", @state_options, id: "state-field" %>
+<%= combobox_tag "state", [], id: "state-field" %>
 
 <%= button_tag "focusable element" %>

Closing this and the related discussion for now. But happy to reopen if we can get a minimal reproduction on main.

@josefarias josefarias closed this Aug 31, 2024
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