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

Line above pre tags is removed #70

Closed
Tetr4 opened this issue Jan 8, 2016 · 8 comments
Closed

Line above pre tags is removed #70

Tetr4 opened this issue Jan 8, 2016 · 8 comments
Assignees
Labels

Comments

@Tetr4
Copy link

Tetr4 commented Jan 8, 2016

Hi, super praktisches layout 👍

There seems to be an issue with lines above pre tags, when removing blanklines is enabled.
With these settings and code:

compress_html:
  blanklines: true
TEST
<pre>blah</pre>

the "TEST" line gets removed.

@Tetr4
Copy link
Author

Tetr4 commented Jan 8, 2016

I did some testing, and I think the code introduced in #68 caused this issue.

@jnvsor jnvsor added the bug label Jan 11, 2016
@jnvsor jnvsor self-assigned this Jan 11, 2016
@jnvsor
Copy link
Collaborator

jnvsor commented Jan 11, 2016

Aww nuts.

@jnvsor
Copy link
Collaborator

jnvsor commented Jan 15, 2016

@Tetr4 @penibelst I have a "Mostly done" version here: jnvsor@270c3ac

There's still a trailing newline in one of the tests and all my twiddling with booleans fixes one spot and makes it pop up in another. Liquid is whack-a-mole 🔨

Could you guys try it out and see what happens?

@jnvsor jnvsor mentioned this issue Jan 15, 2016
@Tetr4
Copy link
Author

Tetr4 commented Jan 15, 2016

Holy moly 😄 Just tested it, looks fine to me.
Performance and liquid code size is a problem though. I agree with your comment to go back to v3.0.0, too much hassle for such a small inconsistancy.

@jnvsor
Copy link
Collaborator

jnvsor commented Jan 19, 2016

I have 2 branches with different versions. Neither of their output is correct, but they're better than what's there now (As in over 30% faster).

  • The first one has a tendancy to eat newlines before and after pre blocks
  • The second one adds a tendancy to add newlines in strange places (Like the first line of the file, before the doctype. The w3c validator says nothing is wrong here but it still smells bad) It's about 5% faster than the first though.

Both of these changes only affect the aesthetics of the source code, and mostly around <pre> tags. The browser output should be identical to non-blanklines mode (Except javascript without semicolons which will now work where it didn't used to - yay! (Unless you have a pre tag inside your javascript or something))

Note the differences between test/source/blanklines/* and test/expected/blanklines/* in both commits.

@Tetr4 @penibelst could you guys tell me which you prefer? The first has slightly cleaner output and the second is slightly faster, though both of them are a large readability improvement on normal mode (And performance improvement on master)

@Tetr4
Copy link
Author

Tetr4 commented Jan 25, 2016

I like the first one. Browser output looks fine and the code looks fine too. 👍
The second one adds a newline before the doctype, as you mentioned. It also puts a newline after the closing </pre> tag:
This

<pre>
asdf
</pre>TEST

becomes

<pre>
asdf
</pre>
TEST

Though i am not sure if this is intended, but something is fishy there.

@jnvsor
Copy link
Collaborator

jnvsor commented Jan 25, 2016

Output is fishy on the first one too. Example:

The next line has whitespace

<pre></pre>

Becomes

The next line has whitespace
    <pre></pre>

I just figured a 5% speed difference might be worth making a fishy output slightly fishier. If @penibelst agrees I'll submit a PR for the first one

@jnvsor jnvsor closed this as completed in #74 Feb 2, 2016
jnvsor added a commit that referenced this issue Feb 2, 2016
Blanklines: Revert pre changes, add testcases, fix #70
@jnvsor
Copy link
Collaborator

jnvsor commented Feb 2, 2016

Alrighty, went with the first version and pushed a new release. Hope I got the release process right @penibelst

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

No branches or pull requests

2 participants