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

PHP functions to add Stimulus JS attributes #737

Open
19Gerhard85 opened this issue Feb 14, 2023 · 16 comments · May be fixed by #2450
Open

PHP functions to add Stimulus JS attributes #737

19Gerhard85 opened this issue Feb 14, 2023 · 16 comments · May be fixed by #2450
Labels

Comments

@19Gerhard85
Copy link

19Gerhard85 commented Feb 14, 2023

It would be nice to have functions or options to add Stimulus attributes on form widgets.

Something like

$builder->add('country', CountryType::class, [
    'attr' = stimulus_target('path/to/controller', 'targetName')
]);

or

$builder->add('country', CountryType::class, [
    'stimulus_target' = ['path/to/controller', 'targetName']
]);

Same goes for stimulus_action and stimulus_controller

@19Gerhard85
Copy link
Author

Any news on that? @weaverryan @tgalopin

@weaverryan weaverryan transferred this issue from symfony/stimulus-bridge Mar 16, 2023
@weaverryan
Copy link
Member

Hi!

I've transferred the issue. Actually, it should be WebpackEncoreBundle... because that holds the Stimulus functions. But they are likely to move to a new bundle soon... so I've moved it here, which is a closer spot than before :).

I like this idea! The 2nd version probably makes the most sense. I suppose we could also allow a stimulus_controller option on the parent form level, so that when you render the <form> tag, it would render with <form data-controlle="foo">.

You CAN already add targets and controllers via your Twig template. But, tbh, it's a bit ugly.

@19Gerhard85
Copy link
Author

19Gerhard85 commented Mar 17, 2023

@weaverryan Thanks for the feedback.

Indeed, it's ugly, especially when you already have some attributes on a widget or you want to render a form_row.

My team and I currently mixing the assignment of stimulus_* in templates and form types which is already a mess 😄

Another thing to evaluate:
ChoiceTypes with the option expanded => true should add the stimulus_* to each widget (e.g. Checkboxes, Radio Buttons), otherwise it wouldn't make sense 😅

@carsonbot
Copy link

Thank you for this issue.
There has not been a lot of activity here for a while. Has this been resolved?

@19Gerhard85
Copy link
Author

Thank you for this issue. There has not been a lot of activity here for a while. Has this been resolved?

Nope

@carsonbot carsonbot removed the Stalled label Apr 26, 2024
@carsonbot
Copy link

Thank you for this issue.
There has not been a lot of activity here for a while. Has this been resolved?

@19Gerhard85
Copy link
Author

Thank you for this issue. There has not been a lot of activity here for a while. Has this been resolved?

Still nope

@carsonbot carsonbot removed the Stalled label Oct 28, 2024
@smnandre
Copy link
Member

Would you like to work on this feature?

@19Gerhard85
Copy link
Author

Would you like to work on this feature?

I could try 😅 We just need to decide which way we would like to go (see first post)

@smnandre
Copy link
Member

I think the 2nd proposal is better, so i guess something like this

$builder->add('country', CountryType::class, [

// 'attr' => [   ?

     'stimulus_controller' => 'controllerName' ,

    'stimulus_target' => ['path/to/controller' => 'targetName'],
    
    'stimulus_action' => ['controllerName' => 'bar'],
    
//   ],    
    
]);

I think user should have the choice to set them on attr, widget_attr, or row_attr.. this, with some form of callable support, would allow to set it to every choice item with a nice DX.

And regarding syntax, i think we should handle different things..

Controller

//  controller names in path
'stimulus_controller' => 'foo/barBar',

//  controller names in identifier syntax 
'stimulus_controller' => 'foo--bar-baz',

// multiple controllers
'stimulus_controller' => ['foo', 'other'],

// controller with values ? 
'stimulus_controller' => ['foo' => ['bar' => 'Bar?', 'baz' => 'Baz!'],  'bar'],

Regarding values, a callable could be very usefull, and allow setting values dynamically depending of form data.

Target

// target per controller    
'stimulus_target' => ['controllerName' => 'targetName'],

// multiple targets
'stimulus_target' => ['controllerName' => 'targetName', 'otherController' => 'otherTargetName'],

Action

// stimulus syntax
'stimulus_action' => 'click->clipboard#copy',

// action per controller (default event -> action)
'stimulus_action' =>  ['controllerName' => 'copy'],

// action per controller (event -> action)
'stimulus_action' =>  ['controllerName' => ['click' => 'copy']],

// action per controller with values
'stimulus_action' =>  ['controllerName' => ['click' => 'copy']],

// action with parameters  ??

Same things for stimulus_outlets & stimulus_classes ?

@19Gerhard85
Copy link
Author

19Gerhard85 commented Dec 17, 2024

@smnandre
I've started working on a PR 🎉
Do you have any preferences/examples for stimulus_controller with values, classes and targets? 🤔

What do you think about:

'stimulus_controller' => [
    'controllerName' => [
        'values' => [
            'param1' => 'value1'
        ],
        'classes' => [
            'loading' => 'spinner'
        ],
        'targets' => [
            'other' => '.target'
        ]
    ]
]

Syntax for targets:

'stimulus_target' => ['controllerName' => 'targetName'],
'stimulus_target' => ['controllerName' => 'targetName1 targetName2'],
'stimulus_target' => ['controllerName' => ['targetName1', 'targetName2']],
'stimulus_target' => ['controllerName' => 'targetName', 'otherController' => 'otherTargetName'],

Syntax for actions:

'stimulus_action' => 'eventName->controllerName#actionName',
'stimulus_action' => ['controllerName' => 'actionName'],
'stimulus_action' => ['controllerName' => ['eventName' => 'actionName']],
'stimulus_action' => ['controllerName' => ['eventName' => ['actionName' => ['param1' => 'value1']]]]

@smnandre
Copy link
Member

DX

What do you think about:

'stimulus_controller' => [
    'controllerName' => [
        'values' => [
            'param1' => 'value1'
        ],
        'classes' => [
            'loading' => 'spinner'
        ],
        'targets' => [
            'other' => '.target'
        ]
    ]
]

Like very much all your examples except this one

'stimulus_target' => ['controllerName' => 'targetName1 targetName2'],

Let's say "maybe later" and keep the door open? Ok for you ?

What about adding this one too ?

'stimulus_controller' => 'controllerName',

DTO ?

Do we want only array_like structures ? What about some DTO ?

Using a new kind of StimulusAttributes (we'll see later how to connect/generize it) but with no hard dependency to Twig to ease manipulation / serialization / etc.. and probably some restrictions on the keys :)

Stimulus::controller('boo/bar', .. ...)
    ->action(...)
    ->action(...)
    ->target(...)
    ...

@Kocal has still optimization things in its pocket if i'm not wrong.. and i have two or three things i'd like to suggest in to improve DX (& performances too)...

Config

Other question: should we add this on all fields ? Inside attr ? Because i don't know if we should add 5 or 6 stimulus_xxx options on every type, label, field.. :)

PR

Feel free to poke me on Slack, or publish in DRAFT here if you want to iterate over your PR even at start, and have some ongoing review :)

@19Gerhard85
Copy link
Author

'stimulus_target' => ['controllerName' => 'targetName1 targetName2'],

This is basically the same like in doc for the StimulusBundle

<div {{ stimulus_target('controller', 'myTarget secondTarget') }}>Hello</div>
'stimulus_controller' => 'controllerName',

This case is already handled 👍

Other question: should we add this on all fields ? Inside attr ? Because i don't know if we should add 5 or 6 stimulus_xxx options on every type, label, field.. :)
For now I've created a FromTypeExtension to apply the attributes to every widget and/or choice_attr / row_attr.

@19Gerhard85 19Gerhard85 linked a pull request Dec 18, 2024 that will close this issue
@Kocal
Copy link
Member

Kocal commented Dec 18, 2024

Do we want only array_like structures ? What about some DTO ?

Arrays are usually a pain to deal with, and more in this case when you won't be able to use PHPDoc on them. It's like the Twig function stimulus_* we introduced a long time ago, because writting plain HTML attributes were painful.

Using a new kind of StimulusAttributes builder, as you suggested @smnandre, is a good idea to me.

@19Gerhard85
Copy link
Author

@Kocal Can you show me an example or some dummy code how it should work?

@smnandre
Copy link
Member

Let's continue on the PR then :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants