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

Rename option-related variables to reflect vanilla Rails conventions #631

Closed
gazayas opened this issue Oct 24, 2023 · 2 comments · Fixed by #633
Closed

Rename option-related variables to reflect vanilla Rails conventions #631

gazayas opened this issue Oct 24, 2023 · 2 comments · Fixed by #633
Assignees
Labels
enhancement New feature or request

Comments

@gazayas
Copy link
Contributor

gazayas commented Oct 24, 2023

After some discussion in #587, it has become clear that we should use our options like this:

  1. options: Options for buttons/select fields/etc. For example, ["one", "two", "three"].
  2. html_options: The same as the Rails helpers in that we set the class attribute, etc.
  3. other_options: Bullet Train specific options. For example help, hide_label, and require.

Read this comment for more details.

@jagthedrummer suggested that we use either bt_options or fp_options instead of other_options. I think this is a good idea, and that it will be clearer for everyone to understand.

#587

I think we should figure this out before #587, because I foresee this being a pretty big PR and it might be better organize things mentally before merging the disable logic from there.

@gazayas gazayas added the enhancement New feature or request label Oct 24, 2023
@gazayas gazayas self-assigned this Oct 24, 2023
@gazayas
Copy link
Contributor Author

gazayas commented Oct 24, 2023

I made another discovery that has an affect on how we should name our variables.

Our fields (_text_field.html.erb, _email_field.html.erb, etc.) are eventually filtered into bullet_train-themes-light/app/views/themes/light/fields/_field.html.erb. So, say for a text_field, we eventually invoke the Rails form helper here with form.send:

<% # the actual field. %>
<% if partial.field? %>
<%= partial.field %>
<% else %>
<% # e.g. form.text_field(method, options) %>
<%= form.send(helper, method, options) %>
<% end %>

Looking through the Rails form helpers, most of them have the following arguments, so I can see why we use options in _field.html.erb:

def text_field(object_name, method, options = {})
  ...
end

In options here, we were putting the html class in there along with other things. So, @jagthedrummer, you were right in your comment here in that html_options is accomplishing the same thing with options. The problem is...

partial.field?

This isn't the case when partial.field? is true, but we're still using <%= render 'shared/fields/field' ... %> for EVERYTHING, forcing us to conflate options and html_options although they should be separated depending on what partial we call.

Say we super scaffold a text field and a super select field.

> bin/super-scaffold crud Project Team title:string types:super_select

Here are the attributes in _show.html.erb:

<% with_attribute_settings object: @project, strategy: :label do %>
  <%= render 'shared/attributes/text', attribute: :title %>
  <%= render 'shared/attributes/super_select', attribute: :types %>
  <%# 🚅 super scaffolding will insert new fields above this line. %>
<% end %>

As you can see, they're just the same render calls. However, if partial.field? is true we get a different invocation from text_field which doesn't necessarily use (object_name, method, options = {}) for its arguments. For the super select field, it yields true for partial.field? because we pass a block to the render call:

<%= render 'shared/fields/field', form: form, method: method, options: html_options, other_options: other_options do %>
<% content_for :field do %>
<%= content_tag :div, wrapper_options do %>
<%= form.select method, choices, options, html_options %>
<% end %>
<% end %>
<% end %>

This is where html_options comes in.

As you can see here, it's using the Rails select form options helper which has a different set of arguments:

def select(object, method, choices = nil, options = {}, html_options = {}, &block)
  ...
end

Conclusion

We don't have a meaningful way of separating options and html_options depending on the partial because we just put everything in a render call, even though the arguments are different from helper to helper.

I'm still processing how to handle this one. I at least hope we can add comments to the code base to make it easier to understand what's going on.

I also still want to change other_options to bt_options.

@gazayas
Copy link
Contributor Author

gazayas commented Oct 24, 2023

I'm also seeing this in the field partial docs.

Individual field partials might have additional options available based on the underlying Rails form field helper. Links to the documentation for individual form field partials are listed at the end of this page.

We could potentially update this to say:

"Individual field partials might have additional options or different uses for options and html_options based on the underlying Rails form field helper."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant