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

Following Turbo action conventions #35

Open
willtcarey opened this issue Sep 28, 2023 · 2 comments
Open

Following Turbo action conventions #35

willtcarey opened this issue Sep 28, 2023 · 2 comments

Comments

@willtcarey
Copy link

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!

= 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 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_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 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!

@marcoroth
Copy link
Owner

marcoroth commented Oct 15, 2023

Hey @willtcarey, thanks for opening this issue!

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?

@willtcarey
Copy link
Author

@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.

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

No branches or pull requests

2 participants