rap2hpoutre / fast-excel

🦉 Fast Excel import/export for Laravel
MIT License
2.08k stars 246 forks source link

Implement per cell styling for openspout #359

Closed jschram closed 2 months ago

jschram commented 4 months ago

It would be nice to be able to format certain values as numeric, date, etc. To achieve this, DateTime values should be accepted as well, from within Exportable::transformRows. There already is support in openspout for DateTime values (see: Writer\XLSX\Manager\WorksheetManager). So, I've made a contribution to openspout/openspout that allows a full Row to be created from an array with styling per cell. This was merged today and a new version has been tagged. See: https://github.com/openspout/openspout/pull/235

This PR implements this change, allowing users to set the formatting of certain columns (based on their numeric index) when exporting data.

For instance:

(new FastExcel(clone $collection))
    ->setColumnStyles([
        0 => (new Style())->setFormat('mm/dd/yyyy'),
    ])
    ->export($file);
jschram commented 3 months ago

Hi @rap2hpoutre! Is there anything I can do to aid you in reviewing this PR? Please let me know if you need further explanation!

jschram commented 3 months ago

Hi @rap2hpoutre, gentle reminder and honest question: Is this package still being maintained?

potsky commented 3 months ago

Hi @jschram

Can't wait to test this! Do you know if we can handle sheets? We generate files with several sheets and need different custom styles on each sheet.

jschram commented 3 months ago

Hi @potsky

Can't wait to test this! Do you know if we can handle sheets? We generate files with several sheets and need different custom styles on each sheet.

Not quite sure, but I can't see why it wouldn't support multiple sheets. I've changed the way the rows are generated so you can provide an array with styles per column. As long as these column styles match for all sheets, I think you'll be fine. When those have to differ, I think it might need some more work.

Could you test it out and let me know if it does indeed work?

potsky commented 3 months ago

Hi @potsky

Can't wait to test this! Do you know if we can handle sheets? We generate files with several sheets and need different custom styles on each sheet.

Not quite sure, but I can't see why it wouldn't support multiple sheets. I've changed the way the rows are generated so you can provide an array with styles per column. As long as these column styles match for all sheets, I think you'll be fine. When those have to differ, I think it might need some more work.

Could you test it out and let me know if it does indeed work?

Yep I will test your PR. The idea is to have a style different on the first sheet and an other on the second. We could add $sheetIndex as an optional second argument like this if I get your example:

(new FastExcel(clone $collection))
    ->setColumnStyles([
        0 => (new Style())->setFormat('mm/dd/yyyy'),
    ], $sheetIndex)
    ->export($file);

I can make a PR on your PR if you want me to add this, just let me know!

jschram commented 3 months ago

Yep I will test your PR.

Awesome, thanks!

The idea is to have a style different on the first sheet and an other on the second. We could add $sheetIndex as an optional second argument like this if I get your example:

(new FastExcel(clone $collection))
    ->setColumnStyles([
        0 => (new Style())->setFormat('mm/dd/yyyy'),
    ], $sheetIndex)
    ->export($file);

I can make a PR on your PR if you want me to add this, just let me know!

Sounds good. Yeah, the $column_styles property in Exportable could be multi-dimensional. As long as you're careful to pass the right set of column styles when writing a row, it would be a nice and generic solution 👍

potsky commented 3 months ago

@jschram it works! Finally we have found an other way for our multi-sheet so we will not add more feature.

jschram commented 3 months ago

@jschram it works!

Glad to hear, thanks a lot for testing!

jschram commented 3 months ago

So with all checks having passed, and a successful manual test, maybe this PR can be merged now, @rap2hpoutre? 🙏

Would be really great if I can use this change from your repository for my project instead of relying on my fork.

rap2hpoutre commented 2 months ago

Oops, I am really sorry for the super late answer! Thank you for this addition 🙏 🙏 , I juste merged it! I will release it right after.

rap2hpoutre commented 2 months ago

Available in v5.5.0.

Thank you for your patience!

jschram commented 2 months ago

No problem, sometimes life just happens. Don't worry about the delay 👍 Thanks again for merging!