-
-
Notifications
You must be signed in to change notification settings - Fork 33
feat: Add keep_at_rules
option
#485
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
base: master
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,13 +15,17 @@ | |
styles: DocumentStyleMap<'_>, | ||
keep_style_tags: bool, | ||
keep_link_tags: bool, | ||
keep_at_rules: bool, | ||
at_rules: Option<String>, | ||
Stranger6667 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
mode: InliningMode, | ||
) -> Result<(), InlineError> { | ||
let sink = Sink::new( | ||
document, | ||
NodeId::document_id(), | ||
keep_style_tags, | ||
keep_link_tags, | ||
keep_at_rules, | ||
at_rules, | ||
mode, | ||
); | ||
let mut ser = HtmlSerializer::new(writer, styles); | ||
|
@@ -34,6 +38,9 @@ | |
node: NodeId, | ||
keep_style_tags: bool, | ||
keep_link_tags: bool, | ||
// TODO this is not used here but I might need it for simplifying serialization logic below | ||
keep_at_rules: bool, | ||
at_rules: Option<String>, | ||
Stranger6667 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
inlining_mode: InliningMode, | ||
} | ||
|
||
|
@@ -43,13 +50,17 @@ | |
node: NodeId, | ||
keep_style_tags: bool, | ||
keep_link_tags: bool, | ||
keep_at_rules: bool, | ||
at_rules: Option<String>, | ||
inlining_mode: InliningMode, | ||
) -> Sink<'a> { | ||
Sink { | ||
document, | ||
node, | ||
keep_style_tags, | ||
keep_link_tags, | ||
keep_at_rules, | ||
at_rules, | ||
inlining_mode, | ||
} | ||
} | ||
|
@@ -60,6 +71,8 @@ | |
node, | ||
self.keep_style_tags, | ||
self.keep_link_tags, | ||
self.keep_at_rules, | ||
self.at_rules.clone(), | ||
Stranger6667 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
self.inlining_mode, | ||
) | ||
} | ||
|
@@ -114,6 +127,22 @@ | |
|
||
serializer.start_elem(&element.name, &element.attributes, style_node_id)?; | ||
|
||
// TODO this part is the one that I don't like the most | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not 100% sure about this one, even though it is a sound approach. The first thought that I have is that the least surprising thing would be to keep On the other hand, this approach is better for performance, and probably it would not be super surprising for the user anyway if we put everything under one So, in the end, I am leaning towards this approach. A few other notes:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. btw, if the user chose to keep There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's something I had no time to look closer into yet There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given the current benchmark results, the performance hit is negligible, so we can skip this. For fragments, likely we can't really do it, as there is no regular HTML structure generally. Maybe we can return an error when the user sets an option to keep at rules, but uses |
||
if element.name.local == local_name!("head") { | ||
if let Some(at_rules) = &self.at_rules { | ||
if !at_rules.is_empty() { | ||
serializer.start_elem( | ||
&QualName::new(None, ns!(html), local_name!("style")), | ||
&Attributes::default(), | ||
None, | ||
)?; | ||
serializer.write_text(at_rules)?; | ||
serializer | ||
.end_elem(&QualName::new(None, ns!(html), local_name!("style")))?; | ||
} | ||
} | ||
} | ||
|
||
self.serialize_children(serializer)?; | ||
|
||
serializer.end_elem(&element.name)?; | ||
|
@@ -575,6 +604,8 @@ | |
IndexMap::default(), | ||
true, | ||
false, | ||
false, | ||
None, | ||
InliningMode::Document, | ||
) | ||
.expect("Should not fail"); | ||
|
@@ -594,6 +625,8 @@ | |
IndexMap::default(), | ||
false, | ||
false, | ||
false, | ||
None, | ||
InliningMode::Document, | ||
) | ||
.expect("Should not fail"); | ||
|
@@ -613,6 +646,8 @@ | |
IndexMap::default(), | ||
false, | ||
false, | ||
false, | ||
None, | ||
InliningMode::Document, | ||
) | ||
.expect("Should not fail"); | ||
|
@@ -632,6 +667,8 @@ | |
IndexMap::default(), | ||
false, | ||
false, | ||
false, | ||
None, | ||
InliningMode::Document, | ||
) | ||
.expect("Should not fail"); | ||
|
@@ -654,9 +691,13 @@ | |
IndexMap::default(), | ||
false, | ||
false, | ||
false, | ||
None, | ||
InliningMode::Document, | ||
) | ||
.expect("Should not fail"); | ||
assert_eq!(buffer, b"<!DOCTYPE html><html><head></head><body data-foo=\"& "\"></body></html>"); | ||
} | ||
|
||
// TODO adding a test case for `keep_at_rules` would be nice | ||
} |
Uh oh!
There was an error while loading. Please reload this page.