-
-
Notifications
You must be signed in to change notification settings - Fork 84
feat(twig): implements stimulus_action() and stimulus_target() Twig functions, close #119 #124
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
feat(twig): implements stimulus_action() and stimulus_target() Twig functions, close #119 #124
Conversation
moismailzai
left a comment
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.
This looks good to me, just a few thoughts!
moismailzai
left a comment
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.
Overall the proposed syntax / documentation / implementation looks good to me. Seeing some duplicated logic in the methods but I think there may be some more work done in here (eg. #123) so maybe not a good idea to start consolidating logic yet. +1 from me.
weaverryan
left a comment
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 like this a lot! The super advanced syntaxes are ugly, but logical - and they will be rarely used. The simple syntax is... nice and simple :). Good work!
| * @return string | ||
| * @throws \Twig\Error\RuntimeError | ||
| */ | ||
| public function renderStimulusAction(Environment $env, $dataOrControllerName, string $actionName = null, string $eventName = null): string |
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.
Don't forget to expose these new functions up in getFunctions() ;)
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.
OMG yes 😱
I totally forgot about them, usually I write tests for Twig Extensions by creating a fake loader and doing assertions on $twig->render() This way I can test the extension and its integration with Twig.
Thanks! 😄
|
Friendly ping @weaverryan @tgalopin Thanks! |
7806114 to
c022b08
Compare
|
As I don't see any other feedback, I'm going to merge this. Thanks @Kocal! But... would you mind opening a PR to the README to document these? I'd like to have that before we release :) |
|
Thanks @weaverryan, I'm on it! |
|
Opened at #125 |
This PR was merged into the main branch. Discussion ---------- doc: stimulus_action() and stimulus_target() Following #124 and #124 (comment) Rendered: https://github.com/Kocal/webpack-encore-bundle/tree/doc/stimulus-target-and-actions#stimulus_action Commits ------- 1fa2503 doc: stimulus_action() and stimulus_target()
This PR was merged into the main branch. Discussion ---------- doc: stimulus_action() and stimulus_target() Following #124 and symfony/webpack-encore-bundle#124 (comment) Rendered: https://github.com/Kocal/webpack-encore-bundle/tree/doc/stimulus-target-and-actions#stimulus_action Commits ------- 1fa2503 doc: stimulus_action() and stimulus_target()
Hi 👋
This PR is a proposal for #119, which adds
stimulus_action()andstimulus_target()Twig functions.stimulus_action()can be a bit hard to use because it supports many syntaxes, since Stimulus allows to use multiple actions in a singledata-actionattribute and we should takes care of the default event:stimulus_action('controller', 'method')stimulus_action('controller', 'method', 'event){{ stimulus_action({ 'controller': 'method', 'controller-2': ['method', 'method2'], 'controller-3': {'click': 'onClick'}, 'controller-4': ['method', {'click': 'onClick'}, {'window@resize': 'onWindowResize'}], }) }}For
stimulus_target:stimulus_target('controller', 'target')stimulus_target('controller', 'target1 target2'), Stimulus allows multiple targetsstimulus_target({ 'controller1': 'target', 'controller2': 'target2 target3' }), array syntaxUsage:
WDYT? Thanks!
For #119 (comment), I think it should belongs to another PR.