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

Collect stylesheets #46

Merged
merged 11 commits into from
May 29, 2017
Merged

Conversation

cborgolte
Copy link
Contributor

Extracts all <link rel="stylesheet" ...> from every Fragment and collects them.

When fragments are merged together, all referenced stylesheet tags are aggregated in the

section of the resulting html page.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 93.902% when pulling 7823b72 on cborgolte:collect-stylesheets into 855b7b7 on tarent:master.

if string(tag) == "link" && attrHasValue(attrs, "rel", "stylesheet") {
href, _ := getAttr(attrs, "href")
stylesheets = append(stylesheets, href.Val)
logging.Logger.WithField("stylesheet", href.Val).Info()
Copy link
Member

Choose a reason for hiding this comment

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

should be removed (to verbose)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

if string(tag) == "link" && attrHasValue(attrs, "rel", "stylesheet") {
href, _ := getAttr(attrs, "href")
stylesheets = append(stylesheets, href.Val)
logging.Logger.WithField("stylesheet", href.Val).Info()
Copy link
Member

Choose a reason for hiding this comment

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

should be removed (to verbose)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -161,6 +171,12 @@ forloop:
continue
}
}
if string(tag) == "link" && attrHasValue(attrs, "rel", "stylesheet") {
Copy link
Member

Choose a reason for hiding this comment

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

This string(tag) == "link" && attrHasValue(attrs, "rel", "stylesheet") could be extracted in a function e.g. getStylesheet()(link string, isStyleheed bool)` would result in

if href, isStylesheed := getStylesheet(tag, attrs); isStylesheed {
  stylesheets = append(stylesheets, href.Val)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -224,7 +250,9 @@ forloop:
buff.Write(raw)
}

return NewStringFragment(buff.String()), dependencies, nil
f = NewStringFragment(buff.String())
f.(*StringFragment).AddStylesheets(stylesheets)
Copy link
Member

Choose a reason for hiding this comment

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

not so nice, better use a new variable with the concrete type instead of using the interface type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

}

func NewStringFragment(c string) *StringFragment {
return &StringFragment {
content: c,
stylesheets: []string{},
Copy link
Member

Choose a reason for hiding this comment

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

No need to initialize a slice. This just waste memory in case you don't need it:

https://blog.golang.org/slices

That said, a nil slice is functionally equivalent to a zero-length slice, even though it points to nothing. It has length zero and can be appended to, with allocation. As an example, look at the one-liner above that copies a slice by appending to a nil slice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

<link uic-remove rel="stylesheet" href="basic.css">
</head>
<body>
<h1 uic-remove>Basic Web Elements</h1>
<h3 uic-remove>The Header</h3>
<uic-fragment name="header">
<link rel="stylesheet" href="/static/body/basic.css#header">
Copy link
Member

Choose a reason for hiding this comment

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

would be good to also have on testcase with two or more stylesheets in a fragment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.


for _, f := range cntx.Head {
if err := f.Execute(w, cntx.MetaJSON, executeFragment); err != nil {
cntx.collectStylesheets(f)
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this done twice? Because there is also a collectStylesheet(f) call within the executeFragment() closure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The collectStylesheets(f) call within the closure is executed against nested fragments. Here, the stylesheets of the parent fragment are collected.

Copy link
Member

Choose a reason for hiding this comment

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

ok, I understand.

for _, f := range cntx.BodyAttrs {
io.WriteString(w, " ")
cntx.collectStylesheets(f)
Copy link
Member

Choose a reason for hiding this comment

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

no need to support stylesheets in body attributes ;-)

- moved duplicate code into a function
- improved tests
- removed excessive logging
@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 93.895% when pulling f21ccc0 on cborgolte:collect-stylesheets into 855b7b7 on tarent:master.

@smancke
Copy link
Member

smancke commented May 29, 2017

cool, it's all fine for me.

@smancke smancke merged commit 732a5ac into qvest-digital:master May 29, 2017
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.

None yet

3 participants