You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Hi, we're loving this gem. It's got a lot of really great abilities in here.
I spent a little while debugging an issue when I was using one of the actions today. I had a turbo stream template which was rendering several turbo stream actions
= turbo_stream.append dom_id(@model, "list"), ...
= turbo_stream.replace dom_id(@model, "map_pin"), ...-# other turbo streams here
And we wanted to add a new stream for changing an attribute. Turbo power has the set_attribute action for this!
This failed however, and you might be able to guess why. Almost all of the Turbo Power actions use the custom_action_all helper, which sends builds turbo stream tags using the targets attribute. When you use the targets attribute then the Turbo JS switches to using querySelectorAll instead of findElementByID to locate your target. That took us a bit of time but we eventually figured out we needed to prefix our dom_id with a # in order to get everything working.
Turbo has started to establish a convention with the stream helpers where calling an action sets the target attribute, and the *_all helpers sets the targets attributes (e.g. append vs append_all).
I think we could add the same thing in Turbo Power where it makes sense. That would involve making many additional helpers and having set_attribute_all in addition to set_attribute. I saw your rejected PR which would have made this quite a bit easier as we could have passed target/targets ourselves, as needed. In lieu of that, however, adding all versions of these actions might be good for consistency. Would you accept a PR which added these or do you have any other ideas of how to achieve this consistency a little better? The biggest downside I can see besides doubling the API surface is it would be a breaking change for the existing API. Let me know what your thoughts are, please!
The text was updated successfully, but these errors were encountered:
I agree that it currently doesn't feel 100% right, but I'm wondering if we can maybe change this upstream for the upcoming Turbo 8 release.
I still feel like the API with keyword arguments feels so much nicer and more intuitive compared to what we have right now. I'm going to propose something on turbo-rails, before adding more to Turbo Power now. How does that sounds to you?
@marcoroth That sounds great to me. I think I would enjoy the keyword API much better overall. Let me know if you need anything from me to open that issue or if I can help in any way.
Hi, we're loving this gem. It's got a lot of really great abilities in here.
I spent a little while debugging an issue when I was using one of the actions today. I had a turbo stream template which was rendering several turbo stream actions
And we wanted to add a new stream for changing an attribute. Turbo power has the
set_attribute
action for this!= turbo_stream.set_attribute dom_id(@model, "map"), ...
This failed however, and you might be able to guess why. Almost all of the Turbo Power actions use the
custom_action_all
helper, which sends builds turbo stream tags using thetargets
attribute. When you use thetargets
attribute then the Turbo JS switches to usingquerySelectorAll
instead offindElementByID
to locate your target. That took us a bit of time but we eventually figured out we needed to prefix ourdom_id
with a#
in order to get everything working.= turbo_stream.set_attribute "##{dom_id(@model, "map")}", ...
This is a little bit uglier, but it's workable.
Turbo has started to establish a convention with the stream helpers where calling an action sets the
target
attribute, and the*_all
helpers sets thetargets
attributes (e.g.append
vsappend_all
).I think we could add the same thing in Turbo Power where it makes sense. That would involve making many additional helpers and having
set_attribute_all
in addition toset_attribute
. I saw your rejected PR which would have made this quite a bit easier as we could have passedtarget
/targets
ourselves, as needed. In lieu of that, however, addingall
versions of these actions might be good for consistency. Would you accept a PR which added these or do you have any other ideas of how to achieve this consistency a little better? The biggest downside I can see besides doubling the API surface is it would be a breaking change for the existing API. Let me know what your thoughts are, please!The text was updated successfully, but these errors were encountered: