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

Add localStyles plugin, tests and dependencies. #447

Closed
wants to merge 7 commits into from
Closed

Add localStyles plugin, tests and dependencies. #447

wants to merge 7 commits into from

Conversation

strarsis
Copy link
Contributor

This PR adds the localStyles plugin which copies styles from style element to style attribute of the selected elements.

  • run this plugin before the removeStyleElement plugin
  • this plugin won't remove the style element, use removeStyleElement after this plugin
  • run convertStyleToAttrs after this plugin
  • minify styles in style element if necessary before this plugin (minified = "computed" styles)
  • this plugin currently works only with class and id selectors, advanced css selectors (e.g. :nth-child) aren't currently supported
  • inline styles will be written after the styles from style element
  • classes are inlined by the order of their names in class element attribute

@Joeynoh
Copy link

Joeynoh commented Oct 28, 2015

+1!

@GreLI
Copy link
Member

GreLI commented Oct 29, 2015

Why there are styles both in attribute and <style> tag?
What about media queries?
Also, code style check fails.

@strarsis
Copy link
Contributor Author

I ran npm test which doesn't invoke make travis, so jshint was ommited,
hence the build errors, fixed that now.

As mediaqueries and other nested structures cannot be added to style attribute,
they would be preserved in style element in any case for further possible usage.

Initially the styles element was intended to be preserved by this plugin
and removed by removeStyleElement plugin instead.
However, there may be mediaqueries and orphaned selectors left in style element.
What kind of modi would make sense (passed as plugin option)?:

  • Preserve style element completely (current mode).
  • Remove matching selectors, preserve everything else.
  • Remove all selectors except nested structures that cannot be copied to style attribute at all (e.g. mediaqueries).

What should happen to the class attributes/classe names inside that matched?
cleanupAttrs plugin afterwards?

With best regards

@Joeynoh
Copy link

Joeynoh commented Oct 29, 2015

Just my two cents: It makes sense that this plugin does not remove the style element, considering there is already a plugin that does. You can simply run them in sequence. Optionally, this plugin can perhaps call removeStyleElement, but should be set to false by default.

Calling cleanupAttrs afterwards makes sense as well.

@strarsis
Copy link
Contributor Author

When there are mediaquery styles, the svgo end user would disable removeStyleElement anyway, so this shouldn't cause trouble either.

Maybe an extra plugin that just removes styles that aren't used by the classes/ids inside before this one, so there are no leftover styles?

@strarsis
Copy link
Contributor Author

I added style element clean up which will strip any selectors/rules
that matched the elements (by class/id) (and which declarations were copied to style attribute).

The following rules apply to style element clean up now:

  • Preserve nested structures (e.g. media queries) in any case (cannot be inlined).
  • Remove selectors that matched.
  • Remove rules where all its selectors matched.

With best regards

P.S.: Maybe later, when it is stable enough, it could be worth it
moving the clean up part into an extra plugin (cleanupStyles.js)?

@strarsis
Copy link
Contributor Author

Ready to merge?

@GreLI
Copy link
Member

GreLI commented Oct 30, 2015

I haven't looked into it closely yet.
Also, we're using indentation of 4 spaces mostly.

@strarsis
Copy link
Contributor Author

strarsis commented Nov 1, 2015

There seems to be more issues posted concerning styles in svgo.

  • Remove styles with display: none; (hidden layers)
  • Remove styles element when not needed (similar to this plugin)
  • Minify styles element (and inline styles?)

To improve selector support, using a XML parser like cheerio would help
because it offers $(...) which can evaluate more complex selectors.


declarations.forEach(function(declaration) {
declarationAst = { type : 'declaration',
property: declaration.property,
Copy link
Member

Choose a reason for hiding this comment

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

uh-oh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can improve/add comments if necessary. :)

@GreLI
Copy link
Member

GreLI commented Nov 2, 2015

I'm sorry, still don't have much time to look into it. Quite a big pull-request.

@strarsis
Copy link
Contributor Author

strarsis commented Nov 13, 2015

I did a full rewrite and rebranch and will create a new PR instead: #592

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