Skip to content

Conversation

adrianthedev
Copy link
Collaborator

@adrianthedev adrianthedev commented May 26, 2025

Description

Add grouped_options for select, check docs for more info.

image

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

@github-actions github-actions bot added the Enhancement Not necessarily a feature, but something has improved label May 26, 2025
Copy link

qlty-cloud-legacy bot commented May 26, 2025

Code Climate has analyzed commit 99c14a3 and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 2

View more on Code Climate.

@adrienpoly
Copy link
Contributor

Thanks @adrianthedev for drafting this — this would be a great addition to the Select field!

If I understand the proposed API correctly, it requires explicitly specifying grouped_options instead of the current options. That definitely makes the API very clear and explicit.

That said, I was wondering if it might be more idiomatic to continue using the existing options parameter and add a bit of logic under the hood to automatically detect whether the provided options are grouped. IMHO this would make the API simpler and possibly more intuitive.

What do you think?

@Paul-Bob
Copy link
Contributor

Hi @adrienpoly

That said, I was wondering if it might be more idiomatic to continue using the existing options parameter and add a bit of logic under the hood to automatically detect whether the provided options are grouped. IMHO this would make the API simpler and possibly more intuitive.

Could you please share two examples of how you expect to provide options, and how you would expect Avo to group them?

@adrienpoly
Copy link
Contributor

From the grouped_options_for_select api page

These are 2 valid structure for grouped_options:

Array syntax

options = [
 ['North America',
   [['United States','US'],'Canada']],
 ['Europe',
   ['Denmark','Germany','France']]
]

or Hash syntax

options = {
  'North America' => [['United States','US'], 'Canada'],
  'Europe' => ['Denmark','Germany','France']
}

From the options_for_select api page
these are valid options format

  • [["Dollar", "$"], ["Kroner", "DKK"]]
  • [ "VISA", "MasterCard" ]
  • { "Basic" => "$20", "Plus" => "$40" }

In AVO

Field is always defined like that

field :country, as: select, options: options

and inside the component we have a grouped_options?(options) method that returns true if the provided options have one of the 2 grouped_options structure

Then in the current PR implementation we would have something like

  def options
    if grouped_options?(@field.options_for_select)
      grouped_options_for_select(@field.options_for_select, selected: @field.value)
    else
      options_for_select(@field.options_for_select, selected: @field.value)
    end
  end

  def grouped_options?(options)
     ....
  end

@Paul-Bob
Copy link
Contributor

Got it, thanks for the detailed explanation. I think we could build an algorithm to detect that. I'm not sure we can guarantee 100% accuracy across all use cases, but it seems feasible.

Now I wonder, why didn't Rails already do this? Why do they have both options_for_select and grouped_options_for_select? I think the answer is: to keep it stupid simple. Each helper expects specific valid syntaxes, and each developer is expected to call the method that best fits the use case.

They probably could have offered just options_for_select and documented all the ways to pass options, including how to handle grouped vs. simple options. But then the documentation would become exhaustive around that single method. From my experience, the more a method tries to do, especially with "magic" behind the scenes, the more questions arise, and the more misunderstood that method becomes. Simplicity is clarity.

I get your point, but I see only one advantage here, and I'm not sure we can truly call it an advantage. Instead of writing field :country, as: :select, grouped_options: options, we'd write field :country, as: :select, options: options, and Avo would add some magic to figure out whether it's grouped or not.

What's the real benefit of that?

I'm not sure what Adrian's stance is, but my position is to keep things super simple. I'd prefer to build an interface in Avo that delegates directly to Rails, options to options_for_select, and grouped_options to grouped_options_for_select, rather than adding a smart layer that might introduce more confusion than simplification.

Let me know if I'm missing a strong case where this abstraction would be genuinely beneficial.

@adrienpoly
Copy link
Contributor

I agree with your points and it is a good call to point that Rails is keeping this as separate methods

Let me know if I'm missing a strong case where this abstraction would be genuinely beneficial.

The only thing I can think of is that is was the first thing I tried (pass a nested hash to options) without looking into the documentation (RTFM 😄 ).

Copy link
Contributor

This PR has been marked as stale because there was no activity for the past 15 days.

@Paul-Bob Paul-Bob marked this pull request as ready for review June 19, 2025 14:54
@Paul-Bob Paul-Bob merged commit a32cf9c into main Jun 19, 2025
19 of 20 checks passed
@Paul-Bob Paul-Bob deleted the enhancement/grouped-options-in-select branch June 19, 2025 15:18
Copy link
Contributor

This PR has been merged into main. The functionality will be available in the next release.

Please check the release guide for more information.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Not necessarily a feature, but something has improved Stale exempt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants