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

Local styles plugin #454

Closed
wants to merge 6 commits into from
Closed

Local styles plugin #454

wants to merge 6 commits into from

Conversation

strarsis
Copy link
Contributor

@strarsis strarsis commented Nov 13, 2015

This PR adds the local styles plugin.
It is a complete rewrite of previous PR, to make the code simpler and more efficient.

@strarsis
Copy link
Contributor Author

strarsis commented Nov 14, 2015

Hm, jshint fails with build using nodejs release v0.10.40, but with other plugins than this one.
Should I base off from a different commit than current master?

*
* @return {Array} output items
*/
function monkeys(items, callFn, selectFn, recurseFn) {
Copy link
Member

Choose a reason for hiding this comment

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

It seems that recurseFn isn't actually used.

@GreLI
Copy link
Member

GreLI commented Nov 14, 2015

Also, I'd like if styles were transfered just if there is only one match, not if two or more (non-adjanced, adjanced can be grouped). It seems it can be done easy enough in the current implementation.

@strarsis
Copy link
Contributor Author

strarsis commented Nov 14, 2015

"Also, I'd like if styles were transfered just if there is only one match, not if two or more (non-adjanced, adjanced can be grouped). It seems it can be done easy enough in the current implementation."

So this means that when a CSS rule with multiple selectors
is selected because one selector matched /not all of them),
then the whole CSS rule should be cleaned?

Like for example using this rule in input:

.test1, test2 {
    color: blue;
}
.test3 {
    background: red;
}
[...]
<rect class="test1" [...]

The rule is cleaned up completely because .test1 matched?:

.test3 {
    background: red;
}
[...]
<rect class="test1" [...]

@strarsis
Copy link
Contributor Author

strarsis commented Nov 17, 2015

The jshint fails with other files for this PR, should I base it off a different commit that passes the tests?
https://travis-ci.org/svg/svgo/jobs/91121264#L176

@GreLI
Copy link
Member

GreLI commented Nov 17, 2015

No, I don't think it's required. Do it if you want.

It's odd that other builds including master branch passed it.

@strarsis
Copy link
Contributor Author

strarsis commented Nov 17, 2015

OK, so the last thing that needs to be clarified is how matched CSS rules are handled.
So a CSS rule should be cleaned up when any of its selectors matched, right?

@GreLI
Copy link
Member

GreLI commented Nov 17, 2015

I mean remove a rule only if there only one corresponding class in document. E.g.

<svg>
<style>
  .thick { stroke-width: 10px }
</style>
<path class="thick" d="..."/>
</svg>

but not then there are several classes here and there:

<svg>
<style>
  .thick { stroke-width: 10px }
</style>
...
<circle class="thick" .../>
...
<path class="thick" d="..."/>
...etc...
</svg>

var i = 0,
length = items.content.length;

while(i < length) {
Copy link
Member

Choose a reason for hiding this comment

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

Why not just simple for loop?

for (var i = 0; i < items.content.length; i++) {

Modern engines optimize it, so there is no need in caching length = items.content.length.

@strarsis
Copy link
Contributor Author

strarsis commented Nov 21, 2015

Yes, I fixed/changed the loop code now.

Preserving selectors/rules when they are used multiple times is a bit tricky,
the current (not pushed yet) implementation is a bit bloated, I have to simplify it a bit.
I also try to get rid of the global variables and add some further tests for edge cases.

@GreLI
Copy link
Member

GreLI commented Nov 21, 2015

Don't forget to check with «CDATA», empty <style/> tag, complex selectors, etc. Just fixed a couple of issues: fc174aa.

@LeZuse
Copy link

LeZuse commented Sep 5, 2016

Do you guys have any update on this?

@strarsis
Copy link
Contributor Author

strarsis commented Sep 5, 2016

Yes, I rewrote the plugin so that it can use the svgo ast as cheerio ast and juice for cleaning up the styles.
cheerio ast back to svgo ast is not fully working yet, hence no new PR has been created yet.

Edit: The reason why I use juice now in this svgo plugin is that it already solved the quite complicated handling of style cascadation, selectors, etc and comes with tests included.

@strarsis
Copy link
Contributor Author

strarsis commented Sep 6, 2016

OK, the rewrite has been finished: #592

@strarsis strarsis closed this Sep 6, 2016
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.

3 participants