-
-
Notifications
You must be signed in to change notification settings - Fork 357
[Turbo] Add Helper/TurboStream::append() et al. methods #2196
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
Conversation
1fdcd34
to
1e0a0cc
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.
I'm not a Turbo user, but I think it can be useful yes.
Just, can you add some documentation for this new feature please? And also fix failing checks (IMO we can ignore the error found by PHPStan).
Thanks! :)
1e0a0cc
to
025ad71
Compare
Updated |
Great addition imho, but what about multiple streams in a single response? Or is that the Twig solution you are talking about @nicolas-grekas? return new TurboResponse(TurboStream::append($target, $html)); Regarding DX it is not optimal to have this I guess: return new TurboResponse(TurboStream::append($target, $html) . TurboStream::replace($target2, $html2)); Is this something to consider? |
Thanks a lot @nicolas-grekas! More comments tomorrow :) |
@rvmourik concatenation FTW indeed. I don't see a better option. In the end, any alternative would just be a fancy way to do concatenation. |
@nicolas-grekas thanks for the quick reply. I know the TurboResponse is extending the base Response class but if we take a look at the JsonResponse there is also an option to pass an array or a mixed value, when it is mixed in the end is converts it to an ArrayObject. Could the TurboResponse do something similar, always convert the passed stream(s) to be a collection of streams and processing them before passing it to the base Response? Maybe I am overthinking it? 😇 |
If we decide to add some helpers to make using Turbo more user-friendly, what if we all DX and provide a fluent interface to not deal with any internal details? return TurboResponse()
->append($targetId, $html)
->prepend($targetId, $html)
->replace($targetId, $html, $method = 'morph')
->update($targetId, $html, $method = 'morph')
->remove($targetId, $html)
->before($targetId, $html)
->after($targetId, $html)
// useful if the action (append, prepend, etc.) is a PHP variable:
->stream($action, $targetId, $html)
; @rvmourik your example now would look like this: // before
return new TurboResponse(TurboStream::append($target, $html) . TurboStream::replace($target2, $html2));
// after
return new TurboResponse()
->append($target, $html)
->append($target2, $html2)
; There's a final consideration, which is not covered either in the current proposal of this PR. In addition to Adding an optional return TurboResponse()
->append(string|null $targetId, string|null $targets, string $html)
->prepend(string|null $targetId, string|null $targets, string $html)
// ... We could add a return TurboResponse()
->append(string $targetId, string $html)
->appendAll(string $targets, string $html)
->prepend(string $targetId, string $html)
->prependAll(string $targets, string $html)
// ... What do you think? |
I think this is a good suggestion, maybe in a future PR the same can be done with adding a helper for a regular turbo frame. I find my self often doing one of the following: a) creating a twig file only with the frame and HTML in it Thanks all. |
025ad71
to
0bc0541
Compare
Works for me, I added the fluent methods to the TurboStreamResponse class.
You might have missed this in the attached patch: this PR uses only targets, with an "s". That means you have to use |
private static function wrap(string $action, string $target, string $html, string $attr = ''): string | ||
{ | ||
return \sprintf(<<<EOHTML | ||
<turbo-stream action="%s" targets="%s"%s> |
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.
Note the "s" to targets, and the docblock comments: we support only CSS selectors, not pure IDs (CSS selectors are more capable anyway).
0bc0541
to
ebd61ee
Compare
PR ready IMHO (failure unrelated IIUC) |
ebd61ee
to
3b337ca
Compare
All green :) |
Thank you @nicolas-grekas. |
This PR was squashed before being merged into the 2.x branch. Discussion ---------- Add `<twig:Turbo:Stream:*>` components | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | Issues | - | License | MIT This PR provides Twig components for turbo-streams: ```html <twig:Turbo:Stream:Append target="#some-id"> <div>Append this</div> </twig:Turbo:Stream:Append> ``` Sibling PR to #2196 See #2196 (comment) for some background. Commits ------- bfdbb46 CI love 356dbc2 Add `<twig:Turbo:Stream:*>` components
For thoughts, this might help, WDYT?
(Note that I'm also wondering about having a twig component to render streams from the twig side, stay tuned)