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 mergeStyles plugin #1381

Merged
merged 49 commits into from
Mar 27, 2021
Merged

Add mergeStyles plugin #1381

merged 49 commits into from
Mar 27, 2021

Conversation

strarsis
Copy link
Contributor

This PR adds the mergeStyles plugin that merges <style> elements into one,
preserving media queries by the <style> media attribute.

You can already try this feature branch by installing it using this npm command:
npm install github:strarsis/svgo#mergeStyles-plugin
The plugin would also be enabled by default in this PR as it can help with compression by subsequent plugins.

Outdated, closed PR: #1278

@strarsis
Copy link
Contributor Author

Hm, strange, the CI test fails (https://github.com/svg/svgo/pull/1381/checks?check_run_id=2087136821#step:6:469).
I can't reproduce this locally with the same node version.

@TrySound
Copy link
Member

I migrated ast to xast https://github.com/syntax-tree/xast

@strarsis
Copy link
Contributor Author

@TrySound: Oh, this would explain the issue. The docs hadn't been updated yet, right?
https://github.com/svg/svgo/blob/master/docs/how-it-works/en.md#2-svg2js

@TrySound
Copy link
Member

I never updated how-it-works 😅

@strarsis strarsis mentioned this pull request Mar 19, 2021
@strarsis
Copy link
Contributor Author

@TrySound: Ready to merge!

package-lock.json Outdated Show resolved Hide resolved
plugins/mergeStyles.js Outdated Show resolved Hide resolved
plugins/mergeStyles.js Outdated Show resolved Hide resolved
plugins/mergeStyles.js Outdated Show resolved Hide resolved
plugins/mergeStyles.js Outdated Show resolved Hide resolved
plugins/mergeStyles.js Outdated Show resolved Hide resolved
@TrySound
Copy link
Member

I meant revert whole package-lock.json. We are not on npm 7 yet.

@TrySound
Copy link
Member

Still here.

plugins/mergeStyles.js Outdated Show resolved Hide resolved
plugins/mergeStyles.js Outdated Show resolved Hide resolved
plugins/mergeStyles.js Outdated Show resolved Hide resolved
plugins/mergeStyles.js Outdated Show resolved Hide resolved
lib/svgo/config.js Outdated Show resolved Hide resolved
lib/css-tools.js Outdated Show resolved Hide resolved
plugins/mergeStyles.js Outdated Show resolved Hide resolved
const style = styles[styleNo];

if (style.mq) {
const wrappedStyles = `@media ${style.mq} {
Copy link
Member

Choose a reason for hiding this comment

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

\n instead of newlines please

Copy link
Contributor Author

@strarsis strarsis Mar 27, 2021

Choose a reason for hiding this comment

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

Replaced with one line. This has the added benefit of saving some space characters.

`@media ${style.mq}{${style.cssStr}}`

plugins/mergeStyles.js Outdated Show resolved Hide resolved
"version": "7.12.13",
"resolved": "https://registry.npmjs.org/@babel/code-frame/-/code-frame-7.12.13.tgz",
"integrity": "sha512-HV1Cm0Q3ZrpCR93tkWOYiuYIgLxZXZFVG2VgK+MBWjUqZTundupbfx2aXarXuw5Ko5aMcjtJgbSs4vUGBS5v6g==",
"version": "7.12.11",
Copy link
Member

Choose a reason for hiding this comment

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

Why I still see package-lock changes here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔 From where should I revert the package-lock? Should I use the current one from svgo or from a previous commit of this branch? npm install seems to update the package lock on its own.

Copy link
Member

@TrySound TrySound Mar 27, 2021

Choose a reason for hiding this comment

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

git checkout HEAD~48 -- package-lock.json

@TrySound TrySound merged commit 19c77d2 into svg:master Mar 27, 2021
@TrySound
Copy link
Member

Thank you!


<svg id="test" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 100 100">
<style>
</style>
Copy link
Contributor

@yisibl yisibl Mar 28, 2021

Choose a reason for hiding this comment

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

A great job. There are a few tips:

The test case should include:

  <style>/* test */</style>
  <style />
  <style type="text/css"></style>

Copy link
Member

Choose a reason for hiding this comment

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

@yisibl do you want to send PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

OK.

Copy link
Contributor

Choose a reason for hiding this comment

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

@TrySound Do we now have an API to recognize CSS comments?

Copy link
Contributor Author

@strarsis strarsis Mar 28, 2021

Choose a reason for hiding this comment

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

The minifyStyles plugin can handle this before and after this plugin.

Copy link
Member

Choose a reason for hiding this comment

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

What if we merge styles after minifying? I asked to move before first because there was unwanted newlines though @strarsis removed them and maybe it makes sense to merge after now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, merging after minification is nice. The minifier removed all comments and only left the bare minimum.

@yisibl
Copy link
Contributor

yisibl commented Mar 28, 2021

In the next PR, do you plan to remove unused CSS?

@TrySound
Copy link
Member

Maybe eventually

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