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

Fix div nbsp #3024

Merged
merged 7 commits into from
Jun 15, 2019
Merged

Fix div nbsp #3024

merged 7 commits into from
Jun 15, 2019

Conversation

efeskucuk
Copy link
Contributor

@efeskucuk efeskucuk commented Jun 14, 2019

This fixes #3014

Edit (old) : Now this fails because I did not change the tests yet, but if you are recommending that we filter out nbsp and send it through text, then why don't we stop all filtering, space too, and send everything down text() ? Only js tests fail, html outputs are all expected, and adjacent nbsps no longer gets merged, every nbsp is present.

@Conduitry
Copy link
Member

This doesn't feel like the right solution to me. This would only notice that it ought to compress to a nbsp if it were the first character in the text node. And it also wouldn't preserve multiple nbsps. What do you think about instead modifying the regex here so that the presence of even one nbsp marks the text node as not compressible?

@efeskucuk
Copy link
Contributor Author

efeskucuk commented Jun 14, 2019

This doesn't feel like the right solution to me. This would only notice that it ought to compress to a nbsp if it were the first character in the text node. And it also wouldn't preserve multiple nbsps. What do you think about instead modifying the regex here so that the presence of even one nbsp marks the text node as not compressible?

Edit : Summarized at the top comment.

@Conduitry
Copy link
Member

We're using space() and not text() to decrease the size of the compiled component. We previously would have a lot of things where we're inserting text nodes containing "\n\n\t\t\t\t\t\t", which is ugly. See #2258.

What I was suggesting was actually something like #3025. That makes nbsp stop a text node from being compressed to space() in the compiled output.

Copy link
Contributor Author

@efeskucuk efeskucuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're using space() and not text() to decrease the size of the compiled component. We previously would have a lot of things where we're inserting text nodes containing "\n\n\t\t\t\t\t\t", which is ugly. See #2258.

What I was suggesting was actually something like #3025. That makes nbsp stop a text node from being compressed to space() in the compiled output.

Yes, I tried the regex then realized that I can also get rid of space, wanted to ask you if there was a reason space is there.

src/compiler/compile/nodes/Text.ts Outdated Show resolved Hide resolved
@efeskucuk
Copy link
Contributor Author

I am not sure which pr should be given credit at this point, but updated it to your suggestion.

@Conduitry Conduitry merged commit be783c5 into sveltejs:master Jun 15, 2019
@efeskucuk efeskucuk deleted the nbsp-div-fix branch June 16, 2019 07:10
@efeskucuk
Copy link
Contributor Author

Thank you for taking your time to help.

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 this pull request may close these issues.

  is stripped if siblings are dynamic
2 participants