-
-
Notifications
You must be signed in to change notification settings - Fork 83
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
Add Twig function and filter for Stimulus Outlets integration #206
Conversation
README.md
Outdated
For example: | ||
|
||
```twig | ||
<div {{ stimulus_outlet('controller', 'foo', '.selector') }}>Hello</div> |
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 haven't worked with outlets yet (I've only read the docs). If I'm correct, an outlet would always be on the same element as a controller, correct? So something like:
<div {{ stimulus_controller('hello') }} {{ stimulus_outlet('hello', 'foo', '.selector') }}>Hello</div<
Is that correct? If so, we should try to "fit" its syntax into stimulus_controller
somehow - it feels unnecessary to require 2x stimulus_
on the same element. For example, perhaps:
<div {{ stimulus_controller('hello')|stimulus_outlet('foo', '.selector')>
WDYT? Probably it would only make sense to make stimulus_outlet
a filter - never its own function that could be used standalone. Or perhaps you can think of a better syntax. When stimulus_controller
was created, CSS classes & outlets didn't exist... so it's getting kind of busy :p. For example, maybe this is better?
<div {{ stimulus_controller('hello').addOutlet('foo', '.selector')>
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 @weaverryan for your answer (I'm a big fan of SymfonyCasts) !
You're right about the doc, I should have mentioned that an outlet would always be on the same element as a controller.
IMHO, a filter would not be the best thing to do because it would only work when coupled with a controller.
So you're right again : let me try to create a new function in StimulusControllersDto !
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.
Tell me if I'm wrong @weaverryan, but isn't there a mistake in StimulusControllersDto, line 39 ?
$this->values['data-'.$controllerName.'-'.$key.'-class'] = $class;
I guess it should be $this->classes['...
, right ?
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.
Woh, yea, totally! Can you please fix it? It actually doesn't break anything, but we shouldn't leave it that way :)
b4a15fa
to
72301cd
Compare
src/Dto/StimulusControllersDto.php
Outdated
public function __toString(): string | ||
{ | ||
if (0 === \count($this->controllers)) { | ||
return ''; | ||
} | ||
|
||
return rtrim( | ||
'data-controller="'.implode(' ', $this->controllers).'" '. | ||
'data-controller='.implode(' ', $this->controllers). |
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.
We do want the "
still
}, array_keys($this->classes), $this->classes)).' '. | ||
implode(' ', array_map(function (string $attribute, string $value): string { | ||
return $attribute.'='.$this->escapeAsHtmlAttr($value); | ||
}, array_keys($this->outlets), $this->outlets)) |
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.
Can you add a test for this - in StimulusControllersDtoTest
?
README.md
Outdated
@@ -260,6 +260,17 @@ If you have multiple controllers on the same element, you can chain them as ther | |||
</div> | |||
``` | |||
|
|||
If you need to attach an outlet to some controller, you can call the addOutlet() method. |
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.
If you need to attach an outlet to some controller, you can call the addOutlet() method. | |
If you need to attach an [outlet](https://stimulus.hotwired.dev/reference/outlets) | |
to the controller, you can call the `addOutlet()` method. |
README.md
Outdated
For example: | ||
|
||
```twig | ||
<div {{ stimulus_controller('foo').addOutlet('bar', '.bar') }}>Hello</div> |
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.
<div {{ stimulus_controller('foo').addOutlet('bar', '.bar') }}>Hello</div> | |
<div {{ stimulus_controller('chart').addOutlet('outlet-controller', '.css-selector') }}>Hello</div> |
@@ -11,10 +11,13 @@ | |||
|
|||
namespace Symfony\WebpackEncoreBundle\Dto; | |||
|
|||
use Twig\Markup; |
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.
Not needed
README.md
Outdated
For example: | ||
|
||
```twig | ||
<div {{ stimulus_outlet('controller', 'foo', '.selector') }}>Hello</div> |
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.
Woh, yea, totally! Can you please fix it? It actually doesn't break anything, but we shouldn't leave it that way :)
1551c8b
to
34ccb3a
Compare
|
||
$this->outlets['data-'.$this->controllers[0].'-'.$outletName.'-outlet'] = $selector; | ||
|
||
return new Markup($this, 'UTF-8'); |
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.
Why is the Markup
class suddenly needed in this situation?
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 probably doing something wrong but if I don't use this class, the result becomes : data-controller=""controller"" data-controller-foo-outlet="".foo""
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.
Weird. It make me think that, below in __toString()
, when we call $this->escapeAsHtmlAttr($value)
, that is taking .foo
and turning it into ".foo"
... but that seems very odd...
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.
It's also clean when I use the raw filter like this : {{ stimulus_controller('foo').addOutlet('bar', '.bar')|raw }}, but it's certainly not what we need.
Outlets added to StimulusBundle! Use the functions from that bundle instead. Thanks @pbories for getting this rolling Cheers! |
…ker0x) This PR was merged into the main branch. Discussion ---------- Fix incorrect classes assignation and controller rendering Fix incorrect class assignation mentioned by `@pbories` in #206 (comment) Commits ------- e716638 Fix incorrect classes assignation and controller rendering
The Outlets API was introduced in Stimulus release v3.2.0.
This PR intends to ease its integration in Twig templates, just like Stimulus controllers, actions and targets.