-
Notifications
You must be signed in to change notification settings - Fork 85
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
Preserve spacing around text nodes and braces #97
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't really review the logic, but looks good overall!
/// Parse a sequence of tokens until we run into a closing tag | ||
/// html! { <div> Hello World </div> } | ||
/// or a brace | ||
/// html! { <div> Hello World { Braced } </div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Closing curly brace missing?
crates/html-macro/src/tag.rs
Outdated
text += &tt.to_string(); | ||
// Insert necessary space in between the tokens in this text node that we're building. | ||
// In the real browser DOM there's no difference between one space and many, | ||
// so we just insert one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not necessarily true! If you use white-space: pre
or white-space: pre-wrap
CSS rules then white-space will not be collapsed.
https://developer.mozilla.org/en-US/docs/Web/CSS/white-space
Not sure if that could be a problem or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahhhh thanks for catching this!
Luckily the fix here is straightforwards enough.
We'll just look at the spans for the text tokens and see how far apart they are - then insert that much space in between then.
And if their spans are on different lines we'll just insert the right amount of new line characters followed by spacing whatever amount of spacing we need.
Also in text.rs
we'll need to insert the right amount of space and newlines before/after the entire string of text.. vs. right now we just insert a single space
percy/crates/html-macro/src/parser/text.rs
Lines 34 to 36 in edd4122
if should_insert_space_after_text { | |
text += " "; | |
} |
Thanks for catching this - I'll add some more test cases and get this fixed. Then we should be in awesome shape here!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took a look and think there's a better solution.. Going to merge this and open a separate issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Co-Authored-By: chinedufn <frankie.nwafili@gmail.com>
Co-Authored-By: chinedufn <frankie.nwafili@gmail.com>
Co-Authored-By: chinedufn <frankie.nwafili@gmail.com>
Fixes #83
Also refactors a good chunk of the
html-macro
to make it easier to read and a good bit more DRY