Skip to content

Conversation

@jmsche
Copy link
Contributor

@jmsche jmsche commented Jul 18, 2022

Closes #123.

This PR aims to add support for Stimulus CSS Classes (see https://stimulus.hotwired.dev/reference/css-classes).

@jmsche
Copy link
Contributor Author

jmsche commented Aug 11, 2022

Hi @weaverryan, would you mind reviewing this PR soon?

I'd love to use this feature in my projects.

Thanks :)

Copy link
Member

@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

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

Looks good - can you just tweak that one part in the docs?


```twig
<div {{ stimulus_controller('chart', { 'name': 'Likes', 'data': [1, 2, 3, 4] }) }}>
<div {{ stimulus_controller('chart', { 'name': 'Likes', 'data': [1, 2, 3, 4] }, { 'loading': 'spinner' }) }}>
Copy link
Member

Choose a reason for hiding this comment

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

I think we should leave this example alone, then add a 2nd example below. The reason is that this might make it look like passing CSS values is required or important, whereas in most cases, it wouldn't be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review :)

I moved it into a separate example, with & without values.

@jmsche
Copy link
Contributor Author

jmsche commented Oct 18, 2022

Code style should be fixed by #194.

I don't know why Tests are cancelled by GitHub :/

[Edit] CI is fixed in #194 as well; ubuntu-18.04 is deprecated, hence the temporary failure.

Copy link
Member

@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

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

Looks good! I'll merge and then we'll make sure tests are passing now that you've fixed them :)

@weaverryan
Copy link
Member

Thanks @jmsche!

@weaverryan weaverryan merged commit 3cbefbf into symfony:main Oct 18, 2022
@weaverryan weaverryan added the Feature New Feature label Oct 18, 2022
@jmsche jmsche deleted the stimulus-classes branch October 18, 2022 15:20
@jmsche jmsche restored the stimulus-classes branch October 18, 2022 15:21
@jmsche jmsche deleted the stimulus-classes branch October 18, 2022 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature New Feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add classes support to {{ stimulus_controller }}

2 participants