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

Address field: Add super-scaffolding support, document new field and document updating dependent-fields in a form #534

Merged
merged 30 commits into from Oct 3, 2023

Conversation

pascallaliberte
Copy link
Member

@pascallaliberte pascallaliberte commented Sep 8, 2023

Closes #533

This PR adds super-scaffolding support, documents the field and documents the self-updating form mechanism to be used elsewhere in forms.

  • Add the new address_field to the super-scaffolding mechanism
  • Document the new address_field compound field partial, its large support for country and region choices out of the box.
  • Add the address field to the showcase preview
  • Document the attributes/address partial and its display options (one_line)
  • Clarify the options available for the address_field (can it be read-only? will that render all sub-fields read-only?). See the showcase preview
  • Document the new mechanism for auto-updating dependent form fields (e.g. the states/provinces/region field updates based on country_id selected). Resolves Selection in one super select affects options of another. bullet_train-fields#42

@pascallaliberte
Copy link
Member Author

pascallaliberte commented Sep 19, 2023

I'm trying to find a good name, a good file name / title, for the new pattern introduced with the address_field whereby the value of one field updates parts of the form further down (a section or a single field).

The trick uses:

  • Two Stimulus controllers (dependable_controller.js and refresh_fields_controller.js),
  • a turbo_frame around the fields expected to be updated via a URL refetch, and
  • a helper (accept_query_string_override_for(form, method)) to avoid needing to manually clear the field name through a strong_params at the controller-level.

Some names I've got down:

  1. Turbo Reactive Field Changes turbo-reactive-field-changes
  2. Self-Updating Form Fields self-updating-form-fields
  3. Turbo Conditional Field Updates turbo-conditional-field-updates
  4. Magic Dependent Fields magic-dependent-fields

Ideas? @kaspth @jagthedrummer @julianrubisch @gazayas

@gazayas
Copy link
Contributor

gazayas commented Sep 19, 2023

@pascallaliberte I personally like turbo-reactive-field-changes!

Looking at the code here...

<%= turbo_frame_tag id_of_dependent_fields_frame,
class: "block space-y-5",
data: {
'controller': "refresh-fields",
'action': "dependable:updated->refresh-fields#updateFrameFromDependentField turbo:frame-render->refresh-fields#finishFrameUpdate",
'refresh-fields-loading-class': 'opacity-60'
} do
%>

I think the name turbo-reactive-field-changes makes it easy to understand that the target of the change is a turbo frame, and also since we're doing changes with Stimulus controllers, the use of the word reactive here sits well with me too.

Other thoughts

If I would change anything, I might call it turbo-reactive-region-field-changes to be more specific. I know it's two fields, :region_id and :postal_code, but I feel like :postal_code is closely tied to region so we can pass for covering this with simply region in the title (for the sake of being specific concerning what we're changing).

End goal

I'm trying to find a good name, a good file name / title, for the new pattern introduced with the address_field

I'm not sure I understand what the end goal is, do we want a new name for a file that we're adding in bullet_train-core, or a name for the purposes of adding it to the documentation? (Looking at the title of the PR I'm assuming it's mainly for documentation purposes.) Sorry if this was clear in your comment and I missed it. Either way, I like turbo-reactive-field-changes 😃

@pascallaliberte
Copy link
Member Author

@gazayas Thanks for chiming in! I appreciate it.

The end goal: document the pattern for uses outside the address field country > region + postal code field updates.

So I'm looking for naming the pattern, creating a page in the docs with a title and a short-ish slug. Something catchy. This pattern is a Bullet Train first, being a super-scaffold friendly no-config way to update fields on a form based on another field. I also think it's a Rails first, so I think it's worth giving BT a marketing boost from it, and hence giving it a solid name.

I also like "Turbo Reactive Field Changes". One drawback: I think it's missing the magic that accept_query_string_override_for provides, where you don't need to modify the strong_params in the controller to make sure the form can read from a query string param.

It's not just that Turbo is involved. It's that this technique is low-config.

Thoughts?

@gazayas
Copy link
Contributor

gazayas commented Sep 19, 2023

I see, so it's not just specifically for the scope of our address partial, but for the pattern in general.

Good point about accept_query_string_override_for. I'm having a hard time coming up with something that will also get across what this method accomplishes along with everything else being included, but I'll comment again if anything comes to mind!

@jagthedrummer
Copy link
Contributor

Of the proposed names I like "Magic Dependent Fields" the best. Partly because it's the shortest, and partly because I think it best describes what the feature is about, without the name being pegged to how it does it. I'd maybe also drop "Magic" and just call it "Dependent Fields".

@kaspth
Copy link
Contributor

kaspth commented Sep 20, 2023

I'm having a bit of a hard time understanding the underlying API here, it seems like we're missing some wiring somewhere particularly given accept_query_string_override_for. I wish we had a form controller to abstract the dependencies away. Something like this maybe?

<%= form_with model: @tangible_thing, data: { controller: "form" } do |form| %>
  <%# Ensure we fire off a request on change, but route it into the dedicated frame %>
  <%= form.select :country_id, data: { action: "change->form#requestSubmit", turbo_frame: form.refreshed_field_name(:country_id) } %>

  <%= turbo_frame_tag form.refreshed_field_name(:country_id) do %>
    <%= render "addresses/form" %>
  <% end %>
<% end %>

@pascallaliberte
Copy link
Member Author

@kaspth I appreciate this. I'll think it over some more.

@pascallaliberte
Copy link
Member Author

@kaspth In our case, I was looking for a way that would fit a few constraints:

  1. not having to add an attribute to the form wrapping the fields because of super-scaffolding support (in particular in the context of adding the address field to an existing form)
  2. not requiring any changes to the resource controller, such as reading from the query_string params and setting a form object's value
  3. not requiring any validation skipping from side-effects of submitting the form

But the API could be cleaned up, to your credit. I did the following changes:

  • Abstracted accept_query_string_override_for into a _dependent_fields_frame partial
  • Added documentation and clarified down to two concepts (see docs): 1) Dependent Fields Pattern and 2) Dependent Fields Frame
  • Chose the main heading of "Dynamic Forms and Dependent Fields"
  • Changed the Stimulus controllers to match the concepts: dependable_controller.js (unchanged) and dependent_fields_frame_controller.js (from refresh_fields_controller.js).
  • Added a showcase preview for _dependent_fields_frame.

@gazayas, @jagthedrummer: I'm dropping "Turbo" and "Magic" from the name of this feature because of how the two concepts (Dependent Fields Pattern and Dependent Fields Frame) are explained, it really feels more like a sober toolkit than a central innovation. It's also found within a heading of "Dynamic Forms and Dependent Fields" in the docs, for scalability and "obviousness"(?).

Reactions? Rebuttals?

@gazayas
Copy link
Contributor

gazayas commented Sep 26, 2023

@pascallaliberte Sounds good to me! After reading @jagthedrummer's comment too a naming convention that has a focus on dependent makes sense for the logic in place, so I'm currently ok with "Dependent Fields Pattern" and "Dependent Fields Frame"

@kaspth
Copy link
Contributor

kaspth commented Sep 26, 2023

@pascallaliberte great job! I just skimmed the code and docs and it looks a lot clearer to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

We'll move the conditional on the `disabled` property. And we'll also let the `dependent_fields_frame` underlying controller handle disabling the field automatically when the `turbo_frame` awaits updates.

```erb
<%= render "shared/fields/dependent_fields_frame",
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically not relevant to this PR, but I keep thinking I should add a default Form Builder for Bullet Train, so we can get stuff like this out of the box:

<%= form.dependent_fields_frame :heard_from do |controller| %>
  <%= form.text_field :heard_from_other, disabled: form.object&.heard_from != "other",
      data: { "#{controller}-target": "field" %>
<% end %>

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this documentation is really clear!

@pascallaliberte pascallaliberte marked this pull request as ready for review September 28, 2023 15:52
Copy link
Contributor

@jagthedrummer jagthedrummer left a comment

Choose a reason for hiding this comment

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

I really like how this turned out. Great write up of everything. Nice work, @pascallaliberte!

@jagthedrummer jagthedrummer merged commit a37318e into main Oct 3, 2023
5 checks passed
@jagthedrummer jagthedrummer deleted the address-field-add-super-scaffold-and-document branch October 3, 2023 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants