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

{strip} doesn't remove white-spaces after comments {* *} #447

Closed
duzun opened this issue May 22, 2018 · 9 comments
Closed

{strip} doesn't remove white-spaces after comments {* *} #447

duzun opened this issue May 22, 2018 · 9 comments
Assignees

Comments

@duzun
Copy link

duzun commented May 22, 2018

Here is an example:

{strip}
    <br />
    {* some comment *}
    <br />
{/strip}

->

<br />
    <br />

And there are some other edge cases around comments.

@peon501
Copy link

peon501 commented May 26, 2018

jeez all know this. Is this is problem to you :)

@wisskid
Copy link
Contributor

wisskid commented Apr 11, 2020

This happens because smarty recognizes a newline directly following a comment as part of the comment and removes it. This is expected, since Smarty 2 would also do this. E.g.:

<tag1></tag1>
{* insightful comment *}
<tag2></tag2>

Is rendered as:

<tag1></tag1>
<tag2></tag2>

This, however causes the newline before the 4 spaces after the comment in de example above to have disappeared from the text content section. We can't take out any spaces at the beginning of a text content section, since that would remove inline spaces following a tag, e.g.:

{strip}{$amount}{if $showCurrency} EUR{if}{/strip}

would run together as

1337EUR

instead of the expected:

1337 EUR

Maybe we can merge consecutive text content sections before compiling them?

@duzun
Copy link
Author

duzun commented Apr 12, 2020

I understand this is an implementation detail.
But I see no semantic issue if strip would remove these spaces.
Merging consecutive text content sections before compiling them sounds reasonable.

In any case, this issue prevents me from commenting some parts of the templates. It is inconvenient.

@wisskid
Copy link
Contributor

wisskid commented Apr 12, 2020 via email

@duzun
Copy link
Author

duzun commented Apr 14, 2020

Thank you @wisskid !
I've just updated smarty over composer and I see lots of spaces removed, just by upgrading!

If I find any issue or edge case, I'll let you know. It's been a while since I've posted this issue, thus I don't remember all the details.

This PR would definitely change the way we comment in smarty templates :)

@duzun
Copy link
Author

duzun commented Apr 14, 2020

ping @peon501

@duzun
Copy link
Author

duzun commented Apr 15, 2020

It looks great so far!
Can’t reproduce any of the issues related to comments any more.

@wisskid
Copy link
Contributor

wisskid commented Apr 15, 2020 via email

@peon501
Copy link

peon501 commented Apr 16, 2020

@duzun ty for ping I will check it later :)

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

Successfully merging a pull request may close this issue.

4 participants