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

[Turbo] Support custom TurboStreamResponse actions #2298

Merged
merged 1 commit into from
Nov 19, 2024

Conversation

DRaichev
Copy link
Contributor

@DRaichev DRaichev commented Oct 24, 2024

Q A
Bug fix? no
New feature? yes
Issues -
License MIT

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

@carsonbot carsonbot added Feature New Feature Turbo Status: Needs Review Needs to be reviewed labels Oct 24, 2024
@DRaichev DRaichev force-pushed the TurboResponse-generic-actions branch 3 times, most recently from 13122e2 to 8514346 Compare October 24, 2024 08:48
@setineos
Copy link

I think it should also work for <twig:Turbo:Stream:*> components.

src/Turbo/src/Helper/TurboStream.php Outdated Show resolved Hide resolved
src/Turbo/src/Helper/TurboStream.php Outdated Show resolved Hide resolved
@nicolas-grekas
Copy link
Member

I think it should also work for twig:Turbo:Stream:* components.

This can be done by adding a generic <twig:Turbo:Stream> component maybe.

@DRaichev
Copy link
Contributor Author

@nicolas-grekas
I just realised if you pass "action" or "targets" into the attributes array those will conflict with the existing attributes.
I think we should throw an exception if those are present

@DRaichev DRaichev force-pushed the TurboResponse-generic-actions branch 4 times, most recently from 3e22298 to 66f7e36 Compare October 27, 2024 22:44
@smnandre
Copy link
Member

With the incoming <twig:Turbo:Frame> and <twig:Turbo:Stream>, do you think we still need this features ?

Kocal added a commit that referenced this pull request Oct 30, 2024
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
@DRaichev
Copy link
Contributor Author

With the incoming <twig:Turbo:Frame> and <twig:Turbo:Stream>, do you think we still need this features ?

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.

@DRaichev DRaichev force-pushed the TurboResponse-generic-actions branch 2 times, most recently from 23a9740 to 03629f7 Compare October 30, 2024 22:38
@smnandre
Copy link
Member

After some thinking, why don't we rename the custom($action) method in action($name, .... ... ... .. ) ?

@DRaichev
Copy link
Contributor Author

After some thinking, why don't we rename the custom($action) method in action($name, .... ... ... .. ) ?

Yeah, I was thinking about this when I renamed the one in TurboStreamResponse
It should be either custom in both or action in both.

I don't really have a preference, both work, so I leave the choice to you

@smnandre
Copy link
Member

Let's use action in both then (sorry for the late comment)

@DRaichev DRaichev force-pushed the TurboResponse-generic-actions branch from bb8aeed to 488e061 Compare October 31, 2024 19:53
Copy link
Member

@smnandre smnandre left a 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 :)

@DRaichev DRaichev force-pushed the TurboResponse-generic-actions branch 4 times, most recently from 8c46bf3 to 1b1aa6e Compare October 31, 2024 21:01
@DRaichev
Copy link
Contributor Author

Ready to merge

Copy link
Member

@smnandre smnandre left a comment

Choose a reason for hiding this comment

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

Thank you @DRaichev !

@carsonbot carsonbot added Status: Reviewed Has been reviewed by a maintainer and removed Status: Needs Review Needs to be reviewed labels Nov 1, 2024
src/Turbo/CHANGELOG.md Outdated Show resolved Hide resolved
@DRaichev DRaichev force-pushed the TurboResponse-generic-actions branch 2 times, most recently from 4b62702 to 6c3a3b7 Compare November 1, 2024 07:51
@DRaichev
Copy link
Contributor Author

DRaichev commented Nov 1, 2024

Updated the changelog, now ready to merge

Copy link
Member

@smnandre smnandre left a comment

Choose a reason for hiding this comment

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

Perfect!

@DRaichev
Copy link
Contributor Author

@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
now I'm waiting for v2.22 to do the <twig:Turbo:Frame/> and hopefully we can get this as well so I can do the instances of custom turbo actions.

@Kocal
Copy link
Member

Kocal commented Nov 19, 2024

Hey, I rebasing your PR and will merge it in the next minutes.

@Kocal Kocal force-pushed the TurboResponse-generic-actions branch from 6c3a3b7 to d29e4d9 Compare November 19, 2024 19:41
@Kocal
Copy link
Member

Kocal commented Nov 19, 2024

Thank you @DRaichev.

@Kocal Kocal merged commit 9c9f58c into symfony:2.x Nov 19, 2024
62 of 63 checks passed
@smnandre
Copy link
Member

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

@DRaichev
Copy link
Contributor Author

DRaichev commented Nov 20, 2024

@Kocal @smnandre Thanks for the speedy reaction. And thank you for all the work you're doing on this project.
I used to hate to work on UX stuff, but thanks to Turbo and Symfony UX I realised you can have great UX without the convoluted mess. Keep up the good work.

@smnandre
Copy link
Member

Thank you for such a kind message—it really means a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New Feature Status: Reviewed Has been reviewed by a maintainer Turbo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants