refactor: make controller more generic #4181
Conversation
elasticspoon
force-pushed
the
make-input-buttons-generic
branch
from
fe82d10
to
156496c
Compare
take in generic html attributes in helper methods
elasticspoon
force-pushed
the
make-input-buttons-generic
branch
from
156496c
to
f8de752
Compare
| // data-form-input-target="addContainer" to be set on the add container | ||
| addItem(event) { | ||
| const template = this.addTemplateTarget.content.firstElementChild.outerHTML; | ||
| const dest = document.querySelector(event.target.dataset.addDestSelector) || |
There was a problem hiding this comment.
If we're using Stimulus, we should be able to just pass this value directly into the controller via value data attributes: https://stimulus.hotwired.dev/reference/values
There was a problem hiding this comment.
The reason I went with this approach is that I wanted to minimize the amount of coupling (maybe the wrong term? context switching?).
If we set the value with stimulus that means that for each button we create we have to additionally set all those values on controller. The controller is at times on a totally different view and you lose some of that context.
I thought the colocation of most of the behavior on the button helper itself just came out much cleaner.
Ex:
<tbody class='fields'>
<%= render 'partners/individuals_requests/item_request', form: form %>
</tbody>
<div>
<%= input_add_button('Add Another Item', container_selector: '.fields') do %>
<%= render 'partners/individuals_requests/item_request' %>
<% end %>
</div>These two fields are right next to each other, makes total sense what is going on, where the input will get added.
Less clear. Personally, I don't even really having the controller there but I am also pretty opposed to just having it as a data attribute cause Imo those are super easy to lose track of.
# app/views/partners/profiles/edit.html.erb
<%= simple_form_for current_partner, url: partners_profile_path,
data: { controller: "form-input", form-input-add-dest-value: "container_id" },
html: {role: 'form', class: 'form-horizontal'} do |form| %>
<%= form.simple_fields_for :profile, current_partner.profile do |profile_form| %>
<div class="row">
<div class="col-6">
# app/views/partners/profiles/edit/_area_served.html.erb
<%= input_add_button "Add Another County", container_selector: "#served_areas", id: "__add_partner_served_area" do %>
<%= f.simple_fields_for :served_areas do |served_area| %>
<%= render 'served_areas/served_area_fields', f: served_area %>
<% end %>
<% end %>There was a problem hiding this comment.
Hmm... fair enough. I feel like there's a better way to accomplish this, but I can't think of one right now. :)
fix: use Nokogiri in tests Swaps from using regex matching of HTML to using Nokogiri to be less flaky. fix: rename helper method Renames input_remove_button and kin to remove_element_button to better reflect what it is doing. The button does more than add or remove an input, it can add/remove any element.
elasticspoon
force-pushed
the
make-input-buttons-generic
branch
from
7ffe3ce
to
635ac5d
Compare
| // data-form-input-target="addContainer" to be set on the add container | ||
| addItem(event) { | ||
| const template = this.addTemplateTarget.content.firstElementChild.outerHTML; | ||
| const dest = document.querySelector(event.target.dataset.addDestSelector) || |
There was a problem hiding this comment.
Hmm... fair enough. I feel like there's a better way to accomplish this, but I can't think of one right now. :)
Issue #3556
I am working to replace cocoon with existing Stimulus behavior. This PR makes the behavior of the Add and Remove buttons more generic so in the future they can be used to replace cocoon behavior.
I plan to create 3 further PRs:
A PR to refactor the javascript that is left into more reasonable controllers.Not so sure about this one. After looking at it, I think unless the goal is specifically to remove jquery the code would get more complex by moving this behavior to stimulus.Type of change
How Has This Been Tested?
Helper Specs