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

Add Twig function and filter for Stimulus Outlets integration #206

Closed
wants to merge 5 commits into from

Conversation

pbories
Copy link

@pbories pbories commented Feb 13, 2023

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.

README.md Outdated
For example:

```twig
<div {{ stimulus_outlet('controller', 'foo', '.selector') }}>Hello</div>
Copy link
Member

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')>

Copy link
Author

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 !

Copy link
Author

@pbories pbories Feb 23, 2023

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 ?

Copy link
Member

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 :)

public function __toString(): string
{
if (0 === \count($this->controllers)) {
return '';
}

return rtrim(
'data-controller="'.implode(' ', $this->controllers).'" '.
'data-controller='.implode(' ', $this->controllers).
Copy link
Member

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))
Copy link
Member

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.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<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;
Copy link
Member

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>
Copy link
Member

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 :)

@pbories pbories force-pushed the main branch 2 times, most recently from 1551c8b to 34ccb3a Compare March 15, 2023 17:17

$this->outlets['data-'.$this->controllers[0].'-'.$outletName.'-outlet'] = $selector;

return new Markup($this, 'UTF-8');
Copy link
Member

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?

Copy link
Author

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

Copy link
Member

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

Copy link
Author

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.

@weaverryan
Copy link
Member

Outlets added to StimulusBundle! Use the functions from that bundle instead. Thanks @pbories for getting this rolling

Cheers!

@weaverryan weaverryan closed this Jun 29, 2023
weaverryan added a commit that referenced this pull request Sep 26, 2023
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants