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

Allow comments within components / function calls #4001

Closed
tacman opened this issue Feb 26, 2024 · 4 comments · Fixed by #4349
Closed

Allow comments within components / function calls #4001

tacman opened this issue Feb 26, 2024 · 4 comments · Fixed by #4349

Comments

@tacman
Copy link

tacman commented Feb 26, 2024

It is so natural to comment out HTML attributes that it's easy to think that Twig supports this. So easy that even the Symfony UX documentation has an example.

// ...

<div class="toggle-password-container"> // Add "toggle-password-container" or a class that applies position: relative to this container.
    <label for="password">Password</label>
    <input
        id="password"
        name="password"
        type="password"
        {{ stimulus_controller('symfony/ux-toggle-password/toggle-password', {
                {# visibleLabel: 'Show password', // If you want to modify this label. #}
                {# visibleIcon: 'Some svg icon', // If you want to modify this icon. #}
                {# hiddenLabel: 'Hide password', // If you want to modify this label. #}
                {# hiddenIcon: 'Some svg icon', // If you want to modify this icon. #}
                buttonClasses: ['toggle-password-button'], // Add as many classes as you wish. "toggle-password-button" is needed to activate the default CSS.
        }) }}
    >
</div>

// ...

https://symfony.com/bundles/ux-toggle-password/current/index.html#usage-without-symfony-forms

I started to make a PR to fix the docs, but it's not easy to decide where to move it. It'd be awesome if twig allowed this, as I frequently use it for in-line comments as well as disabling attributes.

@eokic
Copy link

eokic commented Sep 26, 2024

Not sure what the best way forward is, but I would also like to point out this extremely painful TwigComponent usecase for us where we have to resort to writing prop explanations above the whole thing instead of inline. It becomes increasingly less readable the more props we have of course.

{# props
  propOne: Lorem ipsum dolor sit amet
  propTwo: Lorem ipsum dolor sit amet
  propThree: Lorem ipsum dolor sit amet
#}
{% props
  propOne = 1, // This is not a valid comment
  propTwo = null, /* nor is this */
  propThree = null, {# nor is this of course #}
%}

@fabpot
Copy link
Contributor

fabpot commented Sep 26, 2024

Allowing {# within {{ or {% looks weird to me. That would be the only time when we allow such embeddings.
I think we need to find another notation for inline comments.

I had a look at Jinja and they don't support it.

Liquid has a syntax though, they use #:

{% liquid
  # this is a comment
  assign topic = 'Learning about comments!'
  echo topic
%}

I think it could work. IIUC, such comments in Liquid must be on their own line.

@tacman
Copy link
Author

tacman commented Sep 26, 2024

@fabpot Thanks for considering this. That would work fine for my common use cases.

@fabpot
Copy link
Contributor

fabpot commented Sep 26, 2024

3 lines of code, 130 lines of tests and docs 😂

@fabpot fabpot closed this as completed in 1e4ceff Sep 27, 2024
smnandre added a commit to symfony/ux that referenced this issue Sep 27, 2024
This PR was squashed before being merged into the 2.x branch.

Discussion
----------

[Doc] Misc updates in TogglePassword docs

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| Issues        | -
| License       | MIT

Twig recently added inline comments (twigphp/Twig#4349) and this doc was used an example of invalid Twig syntax (twigphp/Twig#4001). So, let's fix it.

Commits
-------

11276a0 [Doc] Misc updates in TogglePassword docs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

3 participants