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

Trim all but 1 whitespace jinja condition #671

Closed
GuillaumeGomez opened this issue Apr 21, 2022 · 17 comments · Fixed by #673
Closed

Trim all but 1 whitespace jinja condition #671

GuillaumeGomez opened this issue Apr 21, 2022 · 17 comments · Fixed by #673

Comments

@GuillaumeGomez
Copy link
Collaborator

@vallentin suggested here that we could extend the "trim_all_whitespace" feature to be able to trim all whitespace characters but one.

So currently, we have:

<div>    {%- if foo -%} blabla {%- endif -%} </div>

Which will remove all whitespace characters before and after (by using -) and we have:

<div>    {%+ if foo +%} blabla {%+ endif +%} </div>

Which means that the whitespace should be saved (only useful when the "trim all whitespace" feature is enabled).

The idea would now to add a new character to strip all whitespace characters but one. I propose ~:

<div>    {%~ if foo %} blabla {% endif %} </div>
@djc
Copy link
Collaborator

djc commented Apr 21, 2022

I'm on board with all that, including the proposed operator.

@GuillaumeGomez
Copy link
Collaborator Author

Ok, then I'll make a PR soon. :)

@Kijewski
Copy link
Collaborator

I like that! For readability I'd like it best if a run of whitespaces becomes a single '\n' if it contains an '\n', or a space otherwise.

@GuillaumeGomez
Copy link
Collaborator Author

I'm fine with implementing it this way. Anyone else has an opinion maybe?

@djc
Copy link
Collaborator

djc commented Apr 21, 2022

Yeah, I think that approach makes sense.

@vallentin
Copy link
Collaborator

I'd love, if it is also configurable like #664. So you can configure it through the config file or part of the #[template(...)]. As minifying HTML is part of my rendering pipeline, so if it was possible to enable it globally, without needing to sprinkle ~ everywhere, then that would be lovely :)

@GuillaumeGomez
Copy link
Collaborator Author

Sure. What should we name the config parameter then?

@GuillaumeGomez
Copy link
Collaborator Author

Also, if both "trim_all" and "trim_all_but_one" are enabled, what should we do?

@vallentin
Copy link
Collaborator

Maybe instead of having them be individual. Maybe it might be possible to do whitespace = "trim_all" and whitespace = "trim_all_but_one"

@GuillaumeGomez
Copy link
Collaborator Author

GuillaumeGomez commented Apr 21, 2022

Maybe @djc was right: I should have gone with a string argument for "trim_whitespace" so then we wouldn't have this problem.

EDIT: @vallentin Ah we reached the same conclusion haha.

@djc
Copy link
Collaborator

djc commented Apr 21, 2022

Maybe @djc was right: I should have gone with a string argument for "trim_whitespace" so then we wouldn't have this problem.

Yup!

@GuillaumeGomez
Copy link
Collaborator Author

Ok, then I'll change the parameter as well. :)

@vallentin
Copy link
Collaborator

Maybe @djc was right: I should have gone with a string argument for "trim_whitespace" so then we wouldn't have this problem.

@vallentin Ah we reached the same conclusion haha.

Well, there hasn't been a new release yet. So it's not to late to play with it :)

@djc
Copy link
Collaborator

djc commented Apr 21, 2022

I think the names should be suppress, preserve, and then maybe minimize?

@GuillaumeGomez
Copy link
Collaborator Author

I think a mix between @djc's and @vallentin's propositions would work better. What do you think of:

trim_whitespace = "all" | "all_but_one" | "preserve"

?

@djc
Copy link
Collaborator

djc commented Apr 21, 2022

I still dislike anything "trim" because that's not, conceptually, what it is. We're either suppressing the whitespace from the template or preserving it, and this option is somewhere in between. To my eyes, all_but_one seems pretty ugly, and it kind of begs the question about which one.

I do like the idea of just calling it whitespace by default, to keep things simple and concise.

@GuillaumeGomez
Copy link
Collaborator Author

That makes sense. Then let's go with whitespace and suppress|preserve|minimize.

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

Successfully merging a pull request may close this issue.

4 participants