SheetJS / sheetjs

📗 SheetJS Spreadsheet Data Toolkit -- New home https://git.sheetjs.com/SheetJS/sheetjs
https://sheetjs.com/
Apache License 2.0
35k stars 8k forks source link

Writing Style information into Excel #128

Closed nukulb closed 6 years ago

nukulb commented 9 years ago

I am trying to preserve the style information while I read and then write an excel File. Seems like the style information is read when I read the file, but unfortunately when I write the file its not preserved.

Thoughts on how I can read and write the style information?

            excelFile = XLSX.readFile(file.path, {
                cellStyles: true
            });

            // some processing here!

            XLSX.writeFile(excelFile, fileName, {
                cellStyles: true
            });

@sheetjs After a conversation with @elad (https://github.com/hubba/js-xlsx/commit/5e9bca78f2b0c54242cefc8a358f3151232941ab#commitcomment-8039333) I was informed that the styling information is only read, and not written back to excel.

SheetJSDev commented 9 years ago

@pietersv by "experimenting with an XLSX-specific version" I didn't intend to belittle what you have done. I meant that the ultimate solution will have to support reading XLS and writing XLSX (which is a much larger can of worms)

elad commented 9 years ago

@pietersv, @SheetJSDev Can we use CSS and private extensions for stuff that's not standard?

I recall someone suggesting we use Microsoft's private CSS extensions for these styles (if they're exportable to HTML/CSS using Excel's Save As). That could be one approach, another could be something like -sheetjs-tint: -0.19 etc.

See my earlier comment about possible styles - I don't think the proposed four attributes will be enough, and I'd really like to avoid inventing yet another style language if possible. :)

augurer commented 9 years ago

@pietersv I saw your recent posts and was excited to see you had begun implementing styles for the XLSX format, but quickly discovered your solution depends on at least some libraries for NodeJS (e.g., cheerio). Any chance you might update it to support browser-based execution as well?

Thanks!

pietersv commented 9 years ago

@augurer Let me noodle on that. The only dependency is cheerio which is just used as a shortcut to handcrafting the XML. It uses just four few of its methods: $('<el>') to create a node, $doc('el') to select a node, $doc.append(el) to append and $el.attr() to get/set attribute values. I wonder if there is a way to embed cheerio. Alternative is to rewrite StyleBuilder.js to build strings directly, which is certainly possible as that's how the rest of the code works and is impressively hardcore. I think it should be possible to write four tiny methods that do these four steps, using shortcuts rather than full dom parsing.

augurer commented 9 years ago

@pietersv That would be amazing! I started going down the path of modifying code to make it work in the browser and am currently stuck at cheerio.load(baseXml, { xmlMode: true });. I tried using jQuery's parseXML function, which works (maybe?) but then var $fonts = this.$styles('fonts'); fails with the error "function expected".

I'll table this for now and move on to implementing another feature -- please let me know if you develop a solution as I'd love to bold and center text in cells!

SheetJSDev commented 9 years ago

@pietersv directly writing strings is preferable if you don't expect to inspect the xml structure later (you can avoid that by explicitly storing the necessary metadata, like how the shared string table is built and searched when writing) If you plan on making changes you should store the intermediate strings in an array, manipulate the parts that need to be changed, and then join at the end.

pietersv commented 9 years ago

@SheetJSDev Concur that building the strings directly is better and consistent with rest of codebase.

@augurer For quick updated this now so it falls back to jQuery to build the XML if require and module are not defined, and included it in the main xlsx.js file. The class itself generates XML in the browser, and I ran make test and make dist the test suite, but I haven't tested its ability to generate XLSX in the browser. Message me directly if any issues. Centering cell values is not implemented yet.

@elad Will allow format object to accept either CSF or CSS objects. Will duck type and convert to the current CSF format generated by parseFile() so the following would be almost equivalent. When we settle on a format, can later convert parseFile to store that directly.

{
   font: { name: 'Arial', sz: 12, color: {rgb: "#FFFFAA00"}, bold: true, italic: true, underline: true}, 
   fill: {fgColor: { theme: 3, tint: -0.25; },  patternType: 'solid'}, 
   border: {},
   numFmt: "0.00%;-0.00%;-;@"
}

and

{ 
   font-family: "Arial",
   font-size: "12px", 
   font-weight: bold,
   text-decoration: underline,
   font-style: italic,
   color: "#FFAA00",
   background-color: "dk1", 
   -sheetjs-background-tint: "-0.25",
   -sheetjs-number-format: "0.00%;-0.00%;-;@"
}
augurer commented 9 years ago

@pietersv Thanks a lot, you rock! I'll replace my files and give it a try tomorrow or Sunday.

pietersv commented 9 years ago

@SheetJSDev Figured out the make process and made code edits to select entries in the bits/ folder. I wanted to add a new entry "91_style_builder" but added the class definition to "90_utils" instead.

This passes 15,532 tests in the browser suite with 0 failures. I don't know if the new styles actually get written in the browser, will need to write new tests for that. It passes 11,998 tests tests in the server suite then fails 16 round trip tests on preserving date formats, that'll take more thought.

@augurer added alignment as a fifth attribute, with horizontal, vertical, indent and wrapText as subattributes:

   [ // alignment
        {v: "left", "s": { alignment: {horizontal: "left"}}},
        {v: "left", "s": { alignment: {horizontal: "center"}}},
        {v: "left", "s": { alignment: {horizontal: "right"}}},
        {v: "vertical", "s": { alignment: {vertical: "top"}}},
        {v: "vertical", "s": { alignment: {vertical: "center"}}},
        {v: "vertical", "s": { alignment: {vertical: "bottom"}}},
        {v: "indent", "s": { alignment: {indent: "1"}}},
        {v: "indent", "s": { alignment: {indent: "2"}}},
        {v: "indent", "s": { alignment: {indent: "3"}}},
      {
        v: "In publishing and graphic design, lorem ipsum is a filler text commonly used to demonstrate the graphic elements of a document or visual presentation. ",
        s: { alignment: { wrapText: 1, horizontal: 'right', vertical: 'center', indent: 1}}
      }

The current format object closely parallels the OpenXML, as XML itself has tags for font, fill, numFmt, alignment and border and the subtags parallel as well. So I'm thinking that this is pretty good, as it can express styles in a way that Excel can handle. We're not inventing the style description language, Microsft and the OpenXML standards team did. CSS may still be useful as syntactic sugar, as it will be functionally equivalent.

Will hold off on CSS for now and focus on creating tests and getting this addition up to the high quality standards for this project.

pietersv commented 9 years ago

Thanks to @augurer for identifying cross-platform issues with generating XML on IE. Will drop cheerio/jquery and write strings directly.

augurer commented 9 years ago

@pietersv Yeah, really weird. Here's a fiddle demonstrating the problem in IE 10 and IE 11: http://jsfiddle.net/kd2tvb4v/2/

pietersv commented 9 years ago

Checked in a version that writes XML strings directly (removing dependence on jQuery / cheerio)

The 97 fails are "CSV badness" in .xlsb and .xlsx files. All 22,000 tests pass on a clean install, but am not sure what could have changed that would affect that. From what I can tell, it's because XLSX.util.sheet_to_scv(sheet, opts) is appending a newline at the end.

If I comment out the CSV tests, I get 20131 passing, 1 237 pending, 1 failing. The one remaining fail is 1) should parse test files roo_Bibelbund.ods: Error: timeout of 20000ms exceeded. Not sure what changed, but must be something.

pietersv commented 9 years ago

The roo_Bibelbund.ods is a simple timeout, sometimes it takes less than 20 secs and passes, sometimes more and fails. Maybe we extend the time limit, or move the test to the end.

The CSV tests I think need to be updated. These take 3 minutes to get to, so hard to test. Can provide more detail either here or directly as appropriate.

Otherwise this seems to work in server and browser, in Chrome/OSX and IE/Windows. Will monitor this unofficial branch to see if any cases arise.

pietersv commented 9 years ago

Success. Can get all test results to pass including CSV with a small change allowing backward compatibility to the current writeFile(...) allowing limited styles to survive the read-write.

Longer term will extend the reader to read the <Xf> and other records into the CSF object.

Let me know what it might take to get an accepted pull request. The main branch has a large user base and considers a huge array of issues like dates, localization, etc. so am tentative to merge this too early.

protobi commented 9 years ago

Borders implemented. Styles are passed straight through to OpenXML, allowable values in include

["thin","medium","thick","dotted", "hair", "dashed", "mediumDashed", "dashDot", "mediumDashDot", "dashDotDot","mediumDashDotDot", "slantDashDot"]

Examples are below:

{
            v: "Apple",
            s: {
              border: {
                diagonalUp: 1||true,  // for none use 0, false, null, or undefined 
                diagonalDown: 1||true, // for none use 0, false, null, or undefined 
                top: { style: "dashed", color: {auto: 1}},
                right: { style: "medium", color: {theme: "5"}},
                bottom: { style: "hair", color: {theme: 5, tint: "-0.3"}},
                left: { style: "thin", color: {rgb: "FFFFAA00"}},
                diagonal: {style: "dotted", color: {auto: 1}}
              }
            }
          },
          {
              v: "Pear",
              s: {
                border: {
                  diagonalUp: 1, diagonalDown: 1,
                  top: { style: "dashed", color: {auto: 1}},
                  right: { style: "dotted", color: {theme: "5"}},
                  bottom: { style: "mediumDashed", color: {theme: 5, tint: "-0.3"}},
                  left: { style: "double", color: {rgb: "FFFFAA00"}},
                  diagonal: {style: "hair", color: {auto: 1}}
                }
            }

Next steps are documenting the API and adding test cases.

SheetJSDev commented 9 years ago

@protobi FYI we have a (very incomplete) test case for understanding how row/column styles are represented. The base name is cell_style_simple. Unfortunately it doesn't test features like borders.

paulish commented 9 years ago

@protobi I see you created documentation for styles. Color specification does not describe {auto: 1} case. Also I failed to make borders for merged cells. In my tests only the first cell of the merged range has border.

An example code which demonstrates the problem:

var XLSX = require('xlsx');

var cellRef = XLSX.utils.encode_cell({c: 1, r: 1});
var ws = {
      '!cols': [{
         wpx: 40
      }, {
         wpx: 40
      }, {
         wpx: 40
      }],
      '!merges': [{s: {c: 1, r: 1}, e: {c: 2, r: 2}}],
      '!ref': XLSX.utils.encode_range({s: {c: 0, r: 0}, e: {c: 2, r: 2}})
    };
ws[cellRef] = {
   v: 'test',
   t: 's',
   s: {
      alignment: {horizontal: 'center', vertical: 'center'},
      border: {
        left: {style: 'thin', color: {auto: 1}},
        right: {style: 'thin', color: {auto: 1}},
        top: {style: 'thin', color: {auto: 1}},
        bottom: {style: 'thin', color: {auto: 1}}
      }
   }
}
var wb = {
  SheetNames: ['test'],
  Sheets: {test: ws}
};
XLSX.writeFile(wb, 'borders.xlsx');
pietersv commented 9 years ago

@paulish -- great catches. Added {auto:1} to README, will include in next push.

Looking I see adding borders to merged cells is more involved. When adding a thick border to all sides of a merged area, Excel generates eight border styles: (i.e. top, top+right, right, right+bottom, bottom, bottom+left, left, left+top). And it then applies those styles to each individual cell within the merged area.

And to my great surprise, we can actually handle borders for merged areas within the current code. What you have do is is explicitly set border styles for each cell in the merged area. After merging, only the content of the upper left cell will be visible, but the border styles of the others will appear.

[
        {
          v: "This is a submerged cell",
          s:{
            border: {
              left: {style: 'thick', color: {auto: 1}},
              top: {style: 'thick', color: {auto: 1}},
              bottom: {style: 'thick', color: {auto: 1}}
            }
          }
        },
        {
          v: "Pirate ship",
          s:{
            border: {
              top: {style: 'thick', color: {auto: 1}},
              bottom: {style: 'thick', color: {auto: 1}}
            }
          }
        },
        {
          v: "Sunken treasure",
          s:{
            border: {
              right: {style: 'thick', color: {auto: 1}},
              top: {style: 'thick', color: {auto: 1}},
              bottom: {style: 'thick', color: {auto: 1}}
            }
          }
        }]

And for your specific example case you can take a quick shortcut and just give every cell all four borders

var XLSX = require('xlsx');

var cellRef = XLSX.utils.encode_cell({c: 1, r: 1});
var ws = {
  '!cols': [{
    wpx: 40
  }, {
    wpx: 40
  }, {
    wpx: 40
  }],
  '!merges': [{s: {c: 1, r: 1}, e: {c: 2, r: 2}}],
  '!ref': XLSX.utils.encode_range({s: {c: 0, r: 0}, e: {c: 2, r: 2}})
};
ws[XLSX.utils.encode_cell({c: 1, r: 1})] =
    ws[XLSX.utils.encode_cell({c: 1, r: 2})] =
        ws[XLSX.utils.encode_cell({c: 2, r: 1})] =
            ws[XLSX.utils.encode_cell({c: 2, r: 2})] ={
  v: 'test',
  t: 's',
  s: {
    alignment: {horizontal: 'center', vertical: 'center'},
    border: {
      left: {style: 'thin', color: {auto: 1}},
      right: {style: 'thin', color: {auto: 1}},
      top: {style: 'thin', color: {auto: 1}},
      bottom: {style: 'thin', color: {auto: 1}}
    }
  }
}

var wb = {
  SheetNames: ['test'],
  Sheets: {test: ws}
};
XLSX.writeFile(wb, '/tmp/borders.xlsx');
paulish commented 9 years ago

@pietersv Indeed, each border cell of the merge should be added. Thank you.

paulish commented 9 years ago

@protobi, do you have any plans to add textRotation attribute to cell style?

pietersv commented 9 years ago

@paulish Support for textRotation is now a property of alignment. See below.

          {v: "Up 90", s: {alignment: {textRotation: 90}}},
          {v: "Up 45", s: {alignment: {textRotation: 45}}},
          {v: "Horizontal", s: {alignment: {textRotation: 0}}},
          {v: "Down 45", s: {alignment: {textRotation: 135}}},
          {v: "Down 90", s: {alignment: {textRotation: 180}}},
          {v: "Vertical", s: {alignment: {textRotation: 255}}}
pietersv commented 9 years ago

@paulish I think a utility to iterate over merged cells to apply borders is simple but would belong outside the js-xlsx class, either in the end application or a convenience class like workbook, as js-xlsx which is focused on parsing and writing workbooks rather than tools to author them.

For your app, you might write two for loops, one that iterates over vertical indices and sets left/right cell borders, and one that iterates over horizontal indices and sets top/bottom borders.

paulish commented 9 years ago

@pietersv Thanks for your quick solution for textRotation.

About borders for merged cells, I already solved that in my application using 2 for loops :) I see no need for an utility at the moment but it would be nice to explicitly describe in the documentation that each of the merged cells can have an own border.

iamtennislover commented 9 years ago

@pietersv Thanks for creating https://github.com/protobi/js-xlsx I am using it for styling cell color and it works great (using fill). However, I am not able to do two things if you can help with that, that would be great:

  1. I cannot change the font color using font: {color: {rgb: "C6EFCE"}}, but the color changes using font: {color: {theme: "3"}
  2. Is there anyway to insert comment to a cell? Issue #192
iamtennislover commented 9 years ago

Also @pietersv when I read styled XLSX i see that the font style is missing

function read_test() {
    /* read file */
    var book1 = XLSX.readFile('test-format.xlsx', {cellStyles:true});
    console.log('A1', book1.Sheets.Sheet1.A1);  // Check cell style - Gray fill and White font
    console.log('B1', book1.Sheets.Sheet1.B1);  // Normal cell style - No style
    console.log('C1', book1.Sheets.Sheet1.C1);  // Bad cell style - Red fill and dark red font
    console.log('D1', book1.Sheets.Sheet1.D1);  // Good cell style - Green fill and dark green font
    console.log('E1', book1.Sheets.Sheet1.E1);  // Neutral cell style - yellow fill and dark yellow font
}

read_test()

// Output...
A1 { t: 's',
  v: 'Check cell',
  r: '<t>Check cell</t>',
  h: 'Check cell',
  w: 'Check cell',
  s: { patternType: 'solid', fgColor: { rgb: 'A5A5A5' } } }
B1 { t: 's',
  v: 'Normal',
  r: '<t>Normal</t>',
  h: 'Normal',
  w: 'Normal' }
C1 { t: 's',
  v: 'Bad',
  r: '<t>Bad</t>',
  h: 'Bad',
  w: 'Bad',
  s: { patternType: 'solid', fgColor: { rgb: 'FFC7CE' } } }
D1 { t: 's',
  v: 'Good',
  r: '<t>Good</t>',
  h: 'Good',
  w: 'Good',
  s: { patternType: 'solid', fgColor: { rgb: 'C6EFCE' } } }
E1 { t: 's',
  v: 'Neutral',
  r: '<t>Neutral</t>',
  h: 'Neutral',
  w: 'Neutral',
  s: { patternType: 'solid', fgColor: { rgb: 'FFEB9C' } } }
pietersv commented 9 years ago

@abarik1981 I checked in a change just now fixing an issue with rgb font colors. That said, Excel OpenXML seems to use eight-character ARGB values, where A is the alpha or opacity, so try font: {color: {rgb: "FFC6EFCE"}} instead.

There are two related issues with style, reading them (#52) and writing them (#128). Each are fairly involved, so I'm thinking to keep this fork focused exclusively on #128, and within that, just for xlsx/xlsm files. Then may be able to address #52 if/when this branch gets merged. Toward that end, am currently adding unit tests commensurate with the awesome test suite already here.

pietersv commented 9 years ago

@SheetJSDev Your early point about round trip tests is well taken. The best way to test writing styles is to read them back. And the ability to read fills and number formats already exists. So now extending that parsing to include fonts, borders, alignments. In the process, would like to confer on a few suggested changes.

Presently, the parser

e.g. {v: 3.14, s: { patternType: "solid", bgColor: { rgb: "FFAA00"}, fgColor: "FFFFFF"}}.

For writing styles, I'm thinking that:

e.g. {v: 3.14, s: {fill: { patternType: "solid", bgColor: { theme: 5, tint: -0.20}, fgColor: "FFFFFFFF"}}.

This breaks one existing test describe('should correctly handle styles', ...) which would be revised per new syntax. I don't see how to make this backward compatible, but think this should be acceptable, as it would permit even more styles to be parsed than today, and allows all the styles to be written back. Is that okay?

SheetJSDev commented 9 years ago

@pietersv So the original style object was designed to hold common information like background/foreground color. The original comment that proposed the current design, https://github.com/SheetJS/js-xlsx/issues/36#issuecomment-44472566 , was based on my understanding of how XLS and XLSX store style information.

It's probably better to ignore the current style representation (as in, "break backwards compatibility") and see what it looks like.

protobi commented 9 years ago

Just committed change that preserves styles roundtrip: create -> write -> read -> write.
Updated example.js at https://github.com/protobi/workbook accordingly. For right now this preserves only font, fill and numFmt. Will add borders and alignments soon.

The minimalist regex xml parser in the current code is neat, finally got the rhythm of it.

aravindkoneru commented 9 years ago

Is there an estimate for when protobi's branch will be merged with master? I'd really like to use new style implementation (I'm sure I'm not the only one :smile: ) and it'd be cool to have it merged with master as soon as possible.

pietersv commented 9 years ago

I think we're getting close to a pull request. The most recent checkin includes a round-trip test suite (call mocha test-styles.js) which covers a wide variety of styles, and it passes the existing test suite (22073 passing, 4929 pending) with a couple known exceptions.

The only reason I'm hesitant is simple fear of introducing an unknown bug or missing a key use case into a widely used code base until I hear it's working in practice. In immediate short term, perhaps try using the xlsx.js from the branch or npm install protobi/js-xlsx and let me know either here or direct message?

aravindkoneru commented 9 years ago

@pietersv

Thanks for the update. I'll start playing around with your branch and let you if I find any errors.

pietersv commented 9 years ago

I just checked in additions to read/write borders and alignments as well, so it handles most styles round trip. Notes:

@SheetJS I think this might be ready for a pull request, open to further suggestions.

image

elad commented 9 years ago

@pietersv let me just say that this is really great work and the effort you've put into it is much appreciated!

Just one question, since I use this module in production:

I had to blank out the original tests of styles as they're handled differently now.

Does that mean that the API for reading styles changes? If so, could you please add a note in the documentation to allow for a smooth transition?

SheetJSDev commented 9 years ago

@pietersv Excellent work! That test file is beautiful :)

@elad its unclear whether we will use the proposed XLSX-inspired API, stick with the existing structure, or do something completely different. As we discussed earlier @pietersv is exploring the XLSX space. The fun part (working with XLS and other formats) has yet to start.

pietersv commented 9 years ago

@elad I just updated the tests 'different styles' and 'correct styles' to reflect these changes in API:

@SheetJSDev The js-xlsx library is a fabulous resource, so completely understand the need to maintain its integrity and support for other formats. Just to get up to speed on considerations, is the issue (a) that js-xlsx currently reads fill styles for XLS and so we'd need to store them under s.fill in CSF, or (b) that if this writes additional styles for XLSX it'd also need to write the additional styles for XLS and XLSB? Is there a downside to an API that parallels the OpenXML API for XLSX files in a project titled 'js-xlsx'?

elad commented 9 years ago

@pietersv:

Where colors are specified in the XLSX as theme/tint (and not RGB) they are also also specified that way in in CSF (i.e. RGB is not automatically extracted to fill.raw_rgb)

Ouch, that's actually something I'm relying on. If you're not going to provide the RGB value, could you at least expose a utility function to extract it? I think that logic (theme/tint to RGB) belongs in the library rather than application code and wouldn't want to copy/paste it all over the place...

SheetJSDev commented 9 years ago

@pietersv @elad while theme and tint are useful, they are concepts specific to how XLSX represents the colors. Other formats like the XLML (2003) format only store the final display color.

Take a look at the cell_style_simple test case. The colors are stored with tint in the xlsx version, but not in the xlml styles.

pietersv commented 9 years ago

Excel throws an error ("Excel found unreadable content in 'filename.xlsx'.... Repair?") if we write both theme/tint and rgb to OpenXML, as in <fgColor theme="5" tint="-0.5" rgb="FF00FF00"/> .

But there's no reason we can't read it and store ARGB in the style object. The main branch currently stores it as s.raw_rgb. So then how about

pietersv commented 9 years ago

The latest commit parses theme color and saves it as .rgb (and optionally raw_rgb if opts.WTF)

This now passes the tests different styles and correct styles .

HOWEVER the existing tests were expecting colors without tints applied and somehow getting them. For instance, the test file "cell_style_simple.xlsx" specifies this fill color:

       <patternFill patternType="darkHorizontal">
                <fgColor theme="9" tint="-0.249977111117893"/>
                <bgColor theme="5" tint="0.39997558519241921"/>
       </patternFill>

and the theme 9 ("accent6") is specified as follows:

<a:accent6>
  <a:srgbClr val="F79646"/>
</a:accent6>

So it should expect a base color of "F79646" but with the tinting the color should be a darker red. But it gets "F79646" nonetheless and the test has been passing. Can't quite figure out why.

I tried applying the tints and updating the expected colors, but the function rgb_tint(hex, tint) appears to have a bug, as it returns shades of grey, not the correct color. The greys seem about right brightness given the tint, it's just that they're grey. Some examples are below.

So for now I ignore all tinting when storing colors converted from the theme, and the test passes.

console.log([color.hex, color.tint, rgb_tint(color.hex, color.tint)]);
[ '4BACC6', 0.3999755851924192, '939393' ]
[ '9BBB59', 0.3999755851924192, 'C3C3C3' ]
[ '1F497D', -0.499984740745262, '101010' ]
[ 'F79646', -0.249977111117893, 'B9B9B9' ]
[ 'C0504D', 0.3999755851924192, 'D9D9D9' ]
[ 'EEECE1', -0.249977111117893, 'B3B3B3' ]
[ '8064A2', 0.3999755851924192, 'B3B3B3' ]
[ '1F497D', -0.749992370372631, '080808' ]
[ '9BBB59', -0.249977111117893, '747474' ]
[ 'F79646', -0.249977111117893, 'B9B9B9' ]
[ 'EEECE1', 0.3999755851924192, 'F5F5F5' ]
[ '8064A2', 0.3999755851924192, 'B3B3B3' ]
[ '4F81BD', -0.249977111117893, '3B3B3B' ]
[ '8064A2', -0.249977111117893, '606060' ]
pietersv commented 9 years ago

Sorry, theme colors are saved as .rgb (and optionally raw_rgb if opts.WTF)

elad commented 9 years ago

@pietersv, do you mean that the value in .rgb (or .raw_rgb) for the relevant fields gives a different color than what the user sees?

What I think is very important is being able to get the RGB value that represents the final color the human sees when looking at the Excel sheet.

pietersv commented 9 years ago

Concur we'd want to return the tinted colors that the user would have seen in Excel. It appears that line 3 of bits/45_styutils.js has been interpreting hex RRGGBB colors as RRRRRR before converting. Simple fix.

function hex2RGB(h) {
    var o = h.substr(h[0]==="#"?1:0,6);
    return [parseInt(o.substr(0,2),16),parseInt(o.substr(0,2),16),parseInt(o.substr(0,2),16)];
//should be 
    return [parseInt(o.substr(0,2),16),parseInt(o.substr(2,2),16),parseInt(o.substr(4,2),16)];
}```
pietersv commented 9 years ago

Updated to fix hex2RGB and apply tints. Now s.fill.fgColor.rgb is the theme color with tinting applied and s.fill.fgColor.raw_rgb is the theme color without tinting, for backward compatibility and reference. Also updated the tests to expect the tinted color.

elad commented 9 years ago

Thank you @pietersv :)

m1sta commented 9 years ago

How often are people generating files where they couldn't template the styling/formatting in the native application first?

pietersv commented 9 years ago

A quick note about the proposed extension to CSF , while the style is specified for each cell, the style objects are reused where possible. For instance, one could define a style and reuse it in multiple cells, e.g. {v: 1.61828, s: GOLDARIAL_18} where

var GOLD_ARIAL_18 = {
    fill: { bgColor: {rgb:  "FFFFAA000"}}, 
    font: { family: "Arial", sz: 18, bold: true, color: { rgb: "#FF444444"}}, 
    numFmt: "0.00"
};

Even if the styles are different objects with the same value, the style object is de-duplicated using deepEquals in the StyleBuilder class, and reduced to a style reference in the Excel workbook. Thus one could generate a CSS representation for each style, and vice versa generate a style from a CSS representation.

So we can later extend the format to take a CSS string or object instead, which gets converted the same way:

{ v: 1.6818, s: {
    css: "background-color: #FFAA00, font-family: Arial, font-size: 18, color: 444, font-weight: bold",   
    numFmt: "0.00"
}

or

{v: 1.61828, s: { 
     css: { background-color: "#FFAA00", font-family: "Arial", font-size: 18, color: "#444"}, 
    numFmt: "0.00"
  }
}

Right now each unique style gets its own <xf> entry in Excel. One can go further and really tried to compress each style component, i.e. reusing every single font or border spec, but this may be overkill for now.

SheetJSDev commented 9 years ago

the style objects are reused where possible really tried to compress each style component

Should we just store pointers to the styles array? FYI the shared string table (cell type s in XLSX, record type BrtCellIsst in XLSB, record type LabelSst in XLS) also uses pointers, but those are converted to normal string cells for convenience. We could also do the same for cell formats (storing the index in the format table rather than the actual format) and a few other places.

Also, food for thought: to get the actual spreadsheet object out of a Web Worker, you have to serialize and pass a string across the divide. It's a pain, and the worker currently uses JSON.stringify, which isn't exactly efficient and doesn't round-trip properly (try var x = {a:1}, y = {a:x, b:x}, z = JSON.parse(JSON.stringify(y)); y.a.a = z.a.a = 3; console.log(y.b.a, z.b.a);), so a pointer-based format might be useful

protobi commented 9 years ago

@SheetJSDev Sorry for the delay! This is a great question I spent some time going both ways and here's my current thinking. TL;DR -> I like objects per cell, but we could easily support both.

Styles as objects

Net net, specifying styles as objects in each cell seems more convenient and semantically clearer to a user. Here you can see exactly what the format is for each cell, without having to deference pointers, and can easily tweak styles here and there.

If a lot of cells have exactly the same format, can define a format object and share the pointer. If a lot of cells have different formats, but share the same font and fill? Define a fill and font object and share those pointers. This does make the JSON larger but computers are efficient.

Styles as indexes

Here we would specify each cell's style not as an object, but as an integer that refers to an index in an array of cell styles. Presumably we'd use the object CellXf: [] in the CSF workbook.

This mirrors how Excel XLSX stores the styles internally, keeping integer indexes to styles. Excel goes as step farther and turns keep integer indexes to fonts, fills, alignments, number formats and borders. I don;t think we want to go that extra step.

So for the case of a lot of common styles, e.g. a big report with thousands of rows, we could allow (but not require) the user to specify styles in the CellXf property and refer to them by integer.

var cell_style = (typeof s == 'number') ? workbook.CellXf[s] : s

Currently this seems to be filled with empty styles on read. So we'd have to populate that when reading a workbook, and store the integer reference in the cells.

Round trip

I think we're unlikely to have directed cycles in the references, though can imagine a user trying to construct one:

var workbook = { Sheets: { "Main": {
   "A1": { v: "Larry", s: {  fill:   workbook.Sheets.Main.B1.s.fill }},
   "B1": { v: "Moe", s: {  fill:   workbook.Sheets.Main.C1.s.fill }},
   "C1": { v: "Curly", s: {  fill:   workbook.Sheets.Main.A1.s.fill }}
   }
}
aravindkoneru commented 9 years ago

I've been working with @pietersv's branch for a while to write styles and it seems to be working fine. If there aren't any more known issues and it passes the TravisCI builds, I think it might be time to do a pull request.