-
-
Notifications
You must be signed in to change notification settings - Fork 337
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
[Turbo] Support custom TurboStreamResponse actions #2298
Conversation
13122e2
to
8514346
Compare
I think it should also work for |
This can be done by adding a generic |
@nicolas-grekas |
3e22298
to
66f7e36
Compare
With the incoming |
This PR was squashed before being merged into the 2.x branch. Discussion ---------- [Turbo] Add generic `<Turbo:Stream>` component | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | Issues | #2298 (comment) | License | MIT I added a generic Turbo Stream component. To use it, you can do this: ```twig <twig:Turbo:Stream action="turbo_frame_reload" targets="#count-post" /> ``` The rendering is: ```twig <turbo-stream action="turbo_frame_reload" targets="#count-post"></turbo-stream> ``` Commits ------- f0ab499 [Turbo] Add generic `<Turbo:Stream>` component
I think they are complementary. I'd rather use the twig component syntax only in templates and stick to this helper on the php side, plus it's better to have consistent functionality in both, especially for people who dislike the twig component syntax. |
23a9740
to
03629f7
Compare
After some thinking, why don't we rename the |
Yeah, I was thinking about this when I renamed the one in TurboStreamResponse I don't really have a preference, both work, so I leave the choice to you |
Let's use action in both then (sorry for the late comment) |
bb8aeed
to
488e061
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last couple of minor suggestions..
Could you update the CHANGELOG in the src/Turbo repository ?
And let's merge this :)
8c46bf3
to
1b1aa6e
Compare
Ready to merge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @DRaichev !
4b62702
to
6c3a3b7
Compare
Updated the changelog, now ready to merge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect!
@smnandre Hey, can we get this merged ? I am in the process of replacing the usage of the old syntax at work. I've already replaced all basic cases, so |
Hey, I rebasing your PR and will merge it in the next minutes. |
6c3a3b7
to
d29e4d9
Compare
Thank you @DRaichev. |
@DRaichev we will probably release a 2.22 in the coming weeks, you can use it with the "2.x-dev" in the meanwhile! Thanks for this PR 😄 |
Thank you for such a kind message—it really means a lot! |
The current implementation of TurboStream & TurboStreamResponse has no generic action (only the predefined turbo actions) and what's more the class is marked as final, which does not allow adding support for more actions. I think the wrap function should be public or have a public function "custom" that exposes the functionality (I've opted for the latter)
I really like this library: https://github.com/marcoroth/turbo_power
It adds additional simple actions. For example I use set/delete attributes to enable/disable UI elements
This change will allow people to use this or similar libraries, or even build their own custom actions if they want to.
There is no impact to the rest of the functionality or DX
There is a minor change to the wrap function, it now accepts an array of attributes instead of a string. It builds the string from the array of attributes, and adds a leading space. This aims to prevent errors, as the previous implementation needs the $attr argument to start with a blank space otherwise it would produce invalid html