Allow disabling of option and button partials

[gazayas]

Closes #571 and #572.

There were two issues at work here:

1. We weren't passing all of the options to each respective field (only select ones like class, value, etc.). Therefore, even if we declared options: {disabled: true}, it wasn't being passed to the Rails form field.
2. options and html_options were being used differently depending on the partial.

Concerning the second issue, here is an example of the date_field partial versus the buttons partial.

bullet_train-core/bullet_train-themes-tailwind_css/app/views/themes/tailwind_css/fields/_date_field.html.erb

Line 23 in 0af4a51

<%= render 'shared/fields/field', form: form, method: method, options: options, other_options: other_options do %>

bullet_train-core/bullet_train-themes-tailwind_css/app/views/themes/tailwind_css/fields/_buttons.html.erb

Line 11 in 0af4a51

<% content = render 'shared/fields/field', form: form, method: method, options: html_options, other_options: other_options do %>

You can see that html_options is passed for buttons, so options: {disabled: true} wasn't working. I touched upon html_options briefly in this comment in #499, and as you can see here options and html_options were being used interchangeably depending on the partial.

The main reason for this is because the variable name for options was already reserved for the options' labels and values. So, for example, if we pry into the button partial against main, we get the following:

[1] pry(#<#<Class:0x00007fb80fba80c8>>)> options
=> {"one"=>"One", "two"=>"Two", "three"=>"Three"}

When reflecting on #499 I had this code in mind, and here we use field_options which adds another name to the mix. You can see in this line too that super_select, buttons and options are handled differently.

https://github.com/bullet-train-co/bullet_train-core/blob/0af4a51e7254b2412ee26509de0ee1d9ab0a9f43/bullet_train-super_scaffolding/lib/scaffolding/transformer.rb#L757C39-L757C39

So, in this PR, I've done my best to unify the names and to make them easier to understand.

Unifying our options name conventions

As far as the buttons partial, we already had option_field_options in the _option partial, so I decided to use button_field_options so it has a similar name. Again, this represents values like "one" "two" "three" in the hash above.

We touch upon html_options briefly in the docs here, but should we just do away with it altogether and just use options and other_options? I'll have to think through this in a little more depth now that we're standardizing on those two. I also would like to do something about field_options in the Transformer to avoid confusion.

Other field partials

There are other fields that we can't currently disable, like the color picker, which should be updated as well.

[gazayas]

Will see what I can do about the failing tests.

[gazayas]

I excluded id here and on line 28 because id is already automatically being populated.

When using multiple buttons, the ids look like this (considering the partial test from the starter repository):
partial_test_single_button_test_one
partial_test_single_button_test_two
partial_test_single_button_test_three

The tests were failing because options[:id] looks like this, and it was overriding the automatically created ids with three of the same id:
partial_test_single_button_test

[kaspth]

Just some small things I was wondering about.

[jagthedrummer]

🤔 Hmmm.... seems like we have a bit of a mess around the word options. I think this PR is a good attempt as lessening the mess, but I'm a little concerned about it being a breaking change for people who are using the buttons, option, or options fields. Lemme think about this some more. Maybe we just go with it and do a larger version bump when we release this? 🤔

[gazayas]

🤔 Hmmm.... seems like we have a bit of a mess around the word options. I think this PR is a good attempt as lessening the mess, but I'm a little concerned about it being a breaking change for people who are using the buttons, option, or options fields. Lemme think about this some more. Maybe we just go with it and do a larger version bump when we release this? 🤔

I'll leave it up to your judgment! Don't want things getting any messier than they are, so whatever it takes to ease the switch to a more uniform use of options-named variables.

[jagthedrummer]

@gazayas, sorry that it's taken me so long to get back to this.

I'd love to see if we can make these changes in a backwards compatible way that would give us a chance to 1) not break people's apps and 2) communicate to them that they need to make some changes.

I made a suggestion in-line with details about the general idea. Do you think that would work?

If so, I think we'd be able to release this change with little ceremony or warning, and then at some point in the future we could plan to remove the deprecation warnings and backward compatibility code when we do a major version bump.

[jagthedrummer]

@gazayas I wonder if we can do some stuff here to be backwards compatible and to let people know that they should change things.

I'm thinking something like this, though I imagine this is incomplete:

Suggested change

	# Initialize options.
	# Initialize options.
	# Start with a bit of handling for backwards compatibility
	if options.is_a?(Array)
	ActiveSupport::Deprecation.warn(
	":options as an Array is deprecated, use :button_field_options instead" \
	"The way to call 'shared/fields/buttons' has changed. See https://bullettrain.co/docs/TODO-WHAT-PAGE-DO-WE-LINK-TO"
	)
	button_field_options = options
	options = {}
	end
	if html_options.is_a?(Hash)
	ActiveSupport::Deprecation.warn(
	":html_options is deprecated, use :other_options instead" \
	"The way to call 'shared/fields/buttons' has changed. See https://bullettrain.co/docs/TODO-WHAT-PAGE-DO-WE-LINK-TO"
	)
	other_options = html_options
	end

[gazayas]

@jagthedrummer
I agree with making the deprecation messages, and that we should explicitly write in the documentation about using the new keywords for the appropriate views. However, I think we should take a different approach on what we choose to deprecate (i.e. - html_options).

I know we most likely have differing views on this, but I'll go ahead and explain what I think about html_options to bring some clarity.

As the documentation states at least concerning Super Select, html_options is a key inherited from the underlying Rails form helper, and you can see this for different form helpers:

https://github.com/rails/rails/blob/23938052acd773fa24068debe56cd892cbf8d868/actionview/lib/action_view/helpers/form_options_helper.rb#L848-L850

https://github.com/rails/rails/blob/23938052acd773fa24068debe56cd892cbf8d868/actionview/lib/action_view/helpers/url_helper.rb#L234-L244

https://github.com/rails/rails/blob/23938052acd773fa24068debe56cd892cbf8d868/actionview/lib/action_view/helpers/url_helper.rb#L357

Also, from the docs:

Because Bullet Train field partials have more responsibilities than the underlying Rails form field helpers, there are also additional options for things like hiding labels, displaying specific error messages, etc. For these options, we pass them separately as other_options. This keeps them separate from the options in options that will be passed directly to the underlying Rails form field helper.

For that reason, I don't think other_options should be affected in any way by this PR, so we shouldn't need it in the deprecation message.

So, now, I'm under the impression that we should define everything as follows:

1. options: In this case, options for buttons, for example ["one", "two", "three"] (like it originally was).
2. html_options: The same as the Rails helpers in that we set the class attribute, etc.
3. other_options: Bullet Train specific options (i.e. - help, hide_label, require)

Concerning html_options, here's some information from the Rails docs:

You can control the form and button behavior with html_options. Most values in html_options are passed through to the button element. For example, passing a :class option within html_options will set the class attribute of the button element.

So, what I'm mainly trying to say is, I think we should use these keywords in the same way that Rails originally uses them. I know that I've written button_field_options here, but now that I'm looking over things I think we should go with the 3 conventions I mentioned above.

I know there was much discussion on this in #499, and it's not a hill I'm willing to die on, I just wanted to share my thoughts on it before moving forward.

If my understanding of options is incorrect though, I'd be glad to go ahead and write the deprecations to reflect the code suggestion you added here.

[jagthedrummer]

@gazayas I now understand what you were saying in that comment on #499 and I think you're right on. Both in that comment and in what you said above.

I think we should use these keywords in the same way that Rails originally uses them.

💯 I couldn't agree more. Let's do that.

I think your breakdown of which thing means what is good and we should add it to the docs. Though... I do wonder if we could disambiguate other_options a little bit by calling it bt_options or fp_options or something with a little more significance and meaning than other.

[gazayas]

@jagthedrummer I definitely prefer bt_options or fp_options, I think it's definitely clearer. I admittedly wasn't too sure what all the differences were before, but I'm glad I took some time yesterday to sift through it. I'll try to make some progress here and ping you when I clean things up.

[jagthedrummer]

Another spot where we could issue some deprecation warnings and implement some backwards compatibility stuff.

[jagthedrummer]

And one more spot for deprecation warning & backwards compatibility.

[jagthedrummer]

I just came back to look at this and as far as I can tell the two original issues that spawned this PR, #571 and #572, were fixed by #892. (Related, I also tried to rebase this PR and there are lots of conflicts. 😬)

But I think we still have some inconsistencies and ambiguity around options, other_options, and html_options. I'll try to dig in on that idea and see what I can uncover. I'll probably end up documenting whatever I find in a new issue, with plenty of links to some of the commentary and code in this PR. And then will cherry-pick what I can from this PR to get started on standardizing. I'll leave this PR open for now, until I have a new issue to link to.