Add background-color to styles #211

madflow · 2016-05-06T20:09:50Z

This is loosely related to #182. This PR introduces setting a background color to the styles definition.

For xlsx and ods.
Only "solid" backgrounds are supported. XLSX supports a whole array of patternType constants (http://www.officeopenxml.com/SSstyles.php, darkGrid, lightTrellis) - these are ignored.
XLSX knows a bgColor element (The color to use for the background of the fill when the type is not solid.) and a fgColor element (The color to use for the the background in solid fills.) - these are just treated as the same. Only a sick, twisted minded MS Office Interop Developer knows why a background color in solid fills is a foreColor. (I am kidding of course).
Some Tests

boxcla · 2016-05-06T20:10:04Z

Verified that @madflow has signed the CLA. Thanks for the pull request!

tegansnyder · 2016-05-17T13:30:16Z

@madflow you just made my day with this comment:

"Only a sick, twisted minded MS Office Interop Developer knows why a background color in solid fills is a foreColor"

adrilo · 2016-05-19T16:54:20Z

src/Spout/Writer/Style/Style.php

@@ -61,6 +62,13 @@ class Style
    /** @var bool Whether the wrap text property was set */
    protected $hasSetWrapText = false;

+    /** @var string Background color */
+    protected $backgroundColor = null;


can this be set to self::DEFAULT_BACKGROUND_COLOR instead of null?

adrilo · 2016-06-20T03:24:51Z

README.md

@@ -125,7 +125,7 @@ The writer always generate CSV files encoded in UTF-8, with a BOM.

 #### Row styling

-It is possible to apply some formatting options to a row. Spout supports fonts as well as alignment styles.
+It is possible to apply some formatting options to a row. Spout supports fonts, background as well as alignment styles.


you'll have to rebase and resolve the conflicts with borders

madflow · 2016-06-23T19:04:37Z

@adrilo Thanks for your support. I rebased and merged the current upstream.

I agree - we should use "best practice / reusing common parts" for the bgColor and fgColor definition and the fills defininition.

This PR does turn out to be more than a quick afterwork hack - so I would appreciate any tips and pushes to the PR branch.

madflow · 2016-07-15T15:39:07Z

It turns out, XLSX seems to preserve two "no background" definitions. I have a hard time finding the actual specification for this... No luck yet.

But LibreOffice does it and openpyxl and probably everyone else.

    <fills count="2">
        <fill><patternFill patternType="none"/></fill>
        <fill><patternFill patternType="gray125"/></fill>
    </fills>

I'll amend the PR.

I guess I will use fills[0] for "no background" and leave the other one as sleeping beauty.

adrilo · 2016-07-15T22:04:30Z

I don't think there's a spec for that. The spec will describe how <fill> works. Then each implementation does its own thing... Microsoft Excel also creates 2 <fill> so I think we should try to copy that :)

adrilo · 2016-07-15T22:17:42Z

Regarding the reuse of the fills, here is some guidance:

In AbstractStyleHelper.php, when a style gets registered, we should extract the fill information and store it separately. What's stored can be a hash representing the fill data. You assign it an id that you set on the style.
Then every time you register a new style, you do the same thing: extract the fill data, hash it, check if it's already stored; if so, get the id and set it on the style; otherwise add the hash to the fills array with an associated id and set this id to the style.
Finally you write the fills content, you can go through the fills array instead of going through the styles.

This is high level but hopefully it can help. BTW, you can do this in another PR, as the changes involved may be tricky

madflow · 2016-08-06T14:03:19Z

This one is getting very stale :/

I commited and pushed the latest changes. My first approach was to amend the AbstractHelper class - but when I was finished I decided that it should got into the XLSX StyleHelper...

Considering, that fonts and borders are both not re-using definitions in the XLSX world at the moment, this seemed correct. There should be more to come.

Wrote better tests
Deleted bgColor from the XLSX definition. Openpyxl does not use it when just a "start" color is supplied, so I decided that they have already solved this problem.
When we decide to introduce an "end" color - we can introduce it again.
Hacked the two default styles by using a seperate counter for the fillIndex.

I am not perfectly happy how it looks at the moment - but it adds imediate business value. (background colors 💃 )

The test code I used for quick copy+paste:
https://gist.github.com/madflow/d134d476f396a7f0753f2bee327008b9

adrilo · 2016-08-10T05:44:15Z

src/Spout/Writer/XLSX/Helper/StyleHelper.php

+        // The other fills are actually registered by setting a background color
+        foreach ($this->registeredFills as $styleId) {
+
+            /** @var  $style Style */


should be /** @var Style $style */

adrilo · 2016-08-10T05:54:12Z

👍 This looks great!

Now regarding your comments:

Wrote better tests

Feel free to do it now or later. I'm comfortable pushing your changes as is.

Deleted bgColor from the XLSX definition. Openpyxl does not use it when just a "start" color is supplied, so I decided that they have already solved this problem. When we decide to introduce an "end" color - we can introduce it again.

Sounds good!

Hacked the two default styles by using a seperate counter for the fillIndex.

Yeah I guess that's fine. Another way to do it is to store the "pattern" and "fgcolor" attributes in an array inside registeredFills. This way you can add the 2 default fills to this array when it gets initialized and you treat all fills the same way when you render the fills content.

Thanks again for this pull request! I'll merge it whenever you tell me you're done with it.

madflow · 2016-08-10T18:19:37Z

I have pushed the changes, squashed the commits and rebased.

adrilo · 2016-08-10T19:15:35Z

Sorry, I just merged a new commit. I did not see you had pushed a new commit... Can you rebase one more time please?

BTW do you know if I can do it for you? It's annoying to have to ask you every time...

Removed default background color, cosmetics Remove default background color reuse bg colors Cosmetics Moved reusing fills to XLSX StyleHelper Tests and inline doc

madflow · 2016-08-10T19:29:53Z

No - problem - I just rebased. I do not think that it is possible as a "Hub" feature. I can give your write access to my fork - but than you would be just doing my work. But - not 100% sure. Would be interesting to know!

adrilo · 2016-08-10T20:11:58Z

Thanks again @madflow !

adrilo reviewed May 19, 2016
View reviewed changes

madflow force-pushed the background-color branch from 79255c0 to 7fb22e4 Compare May 19, 2016 18:34

madflow force-pushed the background-color branch from 7fb22e4 to 8c64e64 Compare June 18, 2016 10:42

adrilo reviewed Jun 20, 2016
View reviewed changes

madflow force-pushed the background-color branch from 58954d7 to 46619c7 Compare June 23, 2016 18:59

madflow mentioned this pull request Jul 15, 2016

Reuse border definitions #276

Closed

madflow force-pushed the background-color branch from 46619c7 to dd56f46 Compare August 5, 2016 18:46

adrilo reviewed Aug 10, 2016
View reviewed changes

madflow force-pushed the background-color branch from 2b024d2 to 9ae0190 Compare August 10, 2016 18:14

madflow force-pushed the background-color branch from 9ae0190 to 910676d Compare August 10, 2016 19:15

Add background-color to styles

910676d

Removed default background color, cosmetics Remove default background color reuse bg colors Cosmetics Moved reusing fills to XLSX StyleHelper Tests and inline doc

adrilo merged commit 584121d into box:master Aug 10, 2016

madflow deleted the background-color branch August 24, 2016 20:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add background-color to styles #211

Add background-color to styles #211

madflow commented May 6, 2016 •

edited

Loading

boxcla commented May 6, 2016

tegansnyder commented May 17, 2016

adrilo May 19, 2016

adrilo Jun 20, 2016

madflow commented Jun 23, 2016 •

edited

Loading

madflow commented Jul 15, 2016 •

edited

Loading

adrilo commented Jul 15, 2016

adrilo commented Jul 15, 2016

madflow commented Aug 6, 2016 •

edited

Loading

adrilo Aug 10, 2016

adrilo commented Aug 10, 2016

madflow commented Aug 10, 2016

adrilo commented Aug 10, 2016

madflow commented Aug 10, 2016 •

edited

Loading

adrilo commented Aug 10, 2016

Add background-color to styles #211

Add background-color to styles #211

Conversation

madflow commented May 6, 2016 • edited Loading

boxcla commented May 6, 2016

tegansnyder commented May 17, 2016

adrilo May 19, 2016

Choose a reason for hiding this comment

adrilo Jun 20, 2016

Choose a reason for hiding this comment

madflow commented Jun 23, 2016 • edited Loading

madflow commented Jul 15, 2016 • edited Loading

adrilo commented Jul 15, 2016

adrilo commented Jul 15, 2016

madflow commented Aug 6, 2016 • edited Loading

adrilo Aug 10, 2016

Choose a reason for hiding this comment

adrilo commented Aug 10, 2016

madflow commented Aug 10, 2016

adrilo commented Aug 10, 2016

madflow commented Aug 10, 2016 • edited Loading

adrilo commented Aug 10, 2016

madflow commented May 6, 2016 •

edited

Loading

madflow commented Jun 23, 2016 •

edited

Loading

madflow commented Jul 15, 2016 •

edited

Loading

madflow commented Aug 6, 2016 •

edited

Loading

madflow commented Aug 10, 2016 •

edited

Loading