Skip to content

Add <twig:Turbo:Stream:*> components #2227

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

Merged
merged 2 commits into from
Oct 2, 2024

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Oct 1, 2024

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

This PR provides Twig components for turbo-streams:

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

@carsonbot carsonbot added Feature New Feature Status: Needs Review Needs to be reviewed labels Oct 1, 2024
Copy link
Member

@Kocal Kocal left a comment

Choose a reason for hiding this comment

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

LGTM, can you add some tests to check that components nicely render please?
Thanks :)

@carsonbot carsonbot added Status: Reviewed Has been reviewed by a maintainer and removed Status: Needs Review Needs to be reviewed labels Oct 1, 2024
@neluba
Copy link

neluba commented Oct 1, 2024

I don't know if the symfony phpstorm plugin works with third party components, but it might incorrectly offer autocompletions for all properties on all components, because they're inherited from TurboStreamComponent.

@nicolas-grekas nicolas-grekas changed the title Add <twig:turbo-stream-*> components Add <twig:Turbo:Stream:*> components Oct 2, 2024
@nicolas-grekas
Copy link
Member Author

I figured out I could use anonymous twig components, so here we are. Tags are now like:

<twig:Turbo:Stream:Append target="#some-id">
    <div>Append this</div>
</twig:Turbo:Stream:Append>

@rvmourik
Copy link

rvmourik commented Oct 2, 2024

I figured out I could use anonymous twig components, so here we are. Tags are now like:

<twig:Turbo:Stream:Append target="#some-id">
    <div>Append this</div>
</twig:Turbo:Stream:Append>

Is there a specific reason why the attribute is called target and in the HTML using targets?

@nicolas-grekas
Copy link
Member Author

Is there a specific reason why the attribute is called target and in the HTML using targets?

Good question. Turbo accepts both target and targets. The former accepts an HTML id, the latter a CSS selector.
IMHO this is provides bad DX. Using a CSS selector should be the only way: when reading #id, it's way clearer that we target an id, and also it's hinting that another CSS selector would work.

I'm for not supporting Turbo's target attribute.
Then, which name should we give to the CSS-selector attribute on our side? target looks the most sensible to me.
We could name it targets, but the "s" feels unnecessary.

@rvmourik
Copy link

rvmourik commented Oct 2, 2024

Is there a specific reason why the attribute is called target and in the HTML using targets?

Good question. Turbo accepts both target and targets. The former accepts an HTML id, the latter a CSS selector. IMHO this is provides bad DX. Using a CSS selector should be the only way: when reading #id, it's way clearer that we target an id, and also it's hinting that another CSS selector would work.

I'm for not supporting Turbo's target attribute. Then, which name should we give to the CSS-selector attribute on our side? target looks the most sensible to me. We could name it targets, but the "s" feels unnecessary.

I think target is the way to go, but maybe elaborate on the target/targets difference in the docs so people don't mistake the target attribute of the component with the HTML target attribute.

Nevertheless, great DX improvements with the two PR's 👍

@nicolas-grekas nicolas-grekas force-pushed the turbo-components branch 7 times, most recently from 8ca1361 to 1c2205c Compare October 2, 2024 10:49
Copy link
Member

@kbond kbond left a comment

Choose a reason for hiding this comment

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

❤️

@nicolas-grekas nicolas-grekas force-pushed the turbo-components branch 2 times, most recently from f9b54e8 to b4aa5c2 Compare October 2, 2024 12:21
@nicolas-grekas
Copy link
Member Author

#2230 should be fixed for tests to be green.

@nicolas-grekas nicolas-grekas force-pushed the turbo-components branch 2 times, most recently from de35442 to be54f18 Compare October 2, 2024 16:36
@nicolas-grekas
Copy link
Member Author

All green :)

@Kocal Kocal force-pushed the turbo-components branch from be54f18 to bfdbb46 Compare October 2, 2024 16:56
@Kocal
Copy link
Member

Kocal commented Oct 2, 2024

PR rebased and conflicts fixed, I merge when the CI is green.

@Kocal
Copy link
Member

Kocal commented Oct 2, 2024

Thank you Nicolas! :)

@Kocal Kocal merged commit 623fe50 into symfony:2.x Oct 2, 2024
62 checks passed
@onEXHovia
Copy link
Contributor

Is there any benefit from these components in a real application? This package can also be used without TwigComponent. Maybe we should be closer to the package https://github.com/hotwired/turbo-rails?

@nicolas-grekas nicolas-grekas deleted the turbo-components branch October 2, 2024 17:09
@nicolas-grekas
Copy link
Member Author

The benefit is structured declaration: you cannot make a typo, the IDE can help you, etc.

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Oct 2, 2024

I don't about the turbo-rails package, so feel free to open issues/PRs with the specific things you think would be useful for Symfony apps. I'd be happy to review them.

@onEXHovia
Copy link
Contributor

The benefit is structured declaration: you cannot make a typo, the IDE can help you, etc.

You right, but twig component not part of twig(I hope this will change) how blade+components in laravel. In projects where not used twig components will have to install +1 package, this could be a problem. Instead, we can use only twig functions:

{% turbo_stream 'apend', dom_id(todo) %}
{% turbo_frame_tag 'target' %}

I don't mind twig components with html syntax, but look at PR twigphp/Twig#3867 (comment) there are some problems with this. My idea is to use standard features twig to cover as many projects as possible.

@smnandre
Copy link
Member

smnandre commented Oct 4, 2024

I love this PR, and this is such a good example of anonymous components exposed by bundles ! (thanks @yceruto again)

(note to myself: I'm thinking we should use this in the documentation to illustrate!)

Thank you @nicolas-grekas ❤️

@smnandre
Copy link
Member

smnandre commented Oct 4, 2024

Also, in my mind there is no "feature" here that would need to be delivered without TwigComponent. Anyone can do something similar with pure Twig if they want (via embeds or macros).. but for those who have the TwigComponent installed, it's a great sugar syntax / DX improvement.

I don't mind twig components with html syntax, but look at PR twigphp/Twig#3867 (comment) there are some problems with this. My idea is to use standard features twig to cover as many projects as possible.

You then can use {{ component('Turbo:Stream', ... ) }} syntax.

It is 100.00% compatible with Twig standard features and will stay this way.

smnandre added a commit that referenced this pull request Feb 22, 2025
…dre)

This PR was merged into the 2.x branch.

Discussion
----------

[Turbo] Document `<twig:Turbo:Stream:*>`

Hi,

I added docs for #2227.

I will document `<twig:Turbo:Stream>` in a second phase.

What do you think? Do you have any suggestions?

Close #2315

Commits
-------

f572ab5 Update src/Turbo/doc/index.rst
79a0180 Remove too many links
8441ec3 Document <twig:Turbo:Stream:*>
symfony-splitter pushed a commit to symfony/ux-turbo that referenced this pull request Feb 22, 2025
…dre)

This PR was merged into the 2.x branch.

Discussion
----------

[Turbo] Document `<twig:Turbo:Stream:*>`

Hi,

I added docs for symfony/ux#2227.

I will document `<twig:Turbo:Stream>` in a second phase.

What do you think? Do you have any suggestions?

Close #2315

Commits
-------

f572ab5e78a Update src/Turbo/doc/index.rst
79a01807bc4 Remove too many links
8441ec3eafc Document <twig:Turbo:Stream:*>
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants