-
Notifications
You must be signed in to change notification settings - Fork 4
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
Merge body attributes #58
Conversation
This test reflects the behavior we are going to fix now. - just for documentation -
When merging attributes of the <body> Element, all existing class attributes will be merged into one. All other existing attributes will be ov overwritten by new ones.
composition/interfaces.go
Outdated
@@ -83,7 +83,7 @@ type Content interface { | |||
Tail() Fragment | |||
|
|||
// The attributes for the body element | |||
BodyAttributes() Fragment | |||
BodyAttributes() []html.Attribute |
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.
Breaking API change, please leave the old method and create BodyAttributesV2 or something
composition/interfaces.go
Outdated
BodyAttributes() Fragment | ||
|
||
// Return the attributes for the body element as array. | ||
BodyAttributesArray() []html.Attribute |
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.
Still breaking the interface here
5996df6
to
4ea38de
Compare
4ea38de
to
a848b09
Compare
composition/content_merge.go
Outdated
@@ -178,7 +202,12 @@ func (cntx *ContentMerge) GetBodyFragmentByName(name string) (Fragment, bool) { | |||
|
|||
func (cntx *ContentMerge) AddContent(c Content, priority int) { | |||
cntx.addHead(c.Head()) | |||
cntx.addBodyAttributes(c.BodyAttributes()) | |||
content2, ok := c.(Content2) |
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 would call it contentV2 to be clear that the naming choice is a result of a api change
composition/interfaces.go
Outdated
@@ -99,6 +99,13 @@ type Content interface { | |||
MemorySize() int | |||
} | |||
|
|||
type Content2 interface { |
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 would call it ContentV2, see above
This fixes the issue, when multiple tags are merged with identical attributes.
'class' attributes are merged together, other duplicate attributes overwrite their predecessors.