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

Handle Stimulus CSS Classes #191

Merged
merged 1 commit into from
Oct 18, 2022
Merged

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?

See [stimulus-bridge](https://github.com/symfony/stimulus-bridge) for more details.

For example:

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