adobe-research / svgObjectModelGenerator

SVG OM Generator & Writer
Apache License 2.0
49 stars 20 forks source link

Optimize CSS Export: extract similar rules into combining CSS block #107

Closed DmitryBaranovskiy closed 9 years ago

DmitryBaranovskiy commented 9 years ago

Tests are pending… Comments are welcome.

dirkschulze commented 9 years ago

You accidentally added your .DS_Store to the patch.

I am a little bit skeptical about the approach to split a class definition into multiple declarations. The main reason is that this is harder to manually edit. You have a style block where the definition of one class is split out into multiple different locations and can not see all relevant styling information at once.

DmitryBaranovskiy commented 9 years ago

I think it’s brackets plugin issue. It didn’t show me .DS_Store files and it used to work before without including them.

DmitryBaranovskiy commented 9 years ago

Isn’t it the whole purpose of this “optimisation”? Or you want to only do it if there is up to 3 rules left and then have one general class name and put everything else into style attribute? I guess this optimisation could be set under some flag.

dirkschulze commented 9 years ago

@DmitryBaranovskiy To the optimization question: Lets take the following example:

.cls1 {
  font-weight: bold;
  fill: red;
}

.cls2 {
  fill: red;
}

<text class="cls1">Text 1</text>
<text class="cls2">Text 2</text>

The above code is how we would export it today. This could be resolved to the following instead:

.cls2, .cls1 {
  fill: red;
}

.cls1 {
    font-weight: bold;
}

<text class="cls1">Text 1</text>
<text class="cls2">Text 2</text>

This is the way how we do it with your patch. Each class tag on the element has exactly one value. No change here. The class definitions take less lines of code as well, that is much better! However, we do not optimize for hand editing. We could also resolve it this way:

.cls1 {
  fill: red;
}

.cls2 {
  font-weight: bold;
}

<text class="cls1 cls2">Text 1</text>
<text class="cls1">Text 2</text>

The generated realizes that there is a common pattern between class definitions and optimizes these. At the end we have categories of classes that the user likely want to have and that are easily hand editable. You can more safely apply a class definition to another element without reorganizing classes and their block definitions. As more elements we have, as better the generated class categories get. Usually authoring tools create quite a lot of elements. On the other hand, it means that an element might be part of more than one class. IMO this can be a benefit as long as the classes are not too specific because the heuristic tries to optimize class definitions too much and we end up with a class for each property definition. To find the right balance is difficult.

IMO we should go with your patch first since it optimizes for file size. This is absolutely beneficial already. I still hope that we can get this kind of heuristic in the future.