spout icon indicating copy to clipboard operation
spout copied to clipboard

Added support for mergeCells, cell height, shrink to fit

Open quamis opened this issue 5 years ago • 17 comments

Added support for mergeCells: // mergeCells (B2:G2), you may use CellHelper::getColumnLettersFromColumnIndex() to convert from "B2" to "[1,2]" $writer->mergeCells([1,2], [6, 2]);

cell height:
    $row->setHeight(30);

shouldShrinkToFit:
    $style->setShouldShrinkToFit();

These changes are implemented for XLSX as that's what I need and test spout on.

quamis avatar May 04 '20 13:05 quamis

Should close https://github.com/box/spout/issues/529#issuecomment-607089153 & https://github.com/box/spout/issues/729

quamis avatar May 04 '20 13:05 quamis

Seems that one test is failing, any ideea why? I'm not familiar with phpunit or travis

quamis avatar May 04 '20 14:05 quamis

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar May 04 '20 14:05 CLAassistant

@quamis From what I can see it's a code style issue. Run the following command in your terminal to fix it: vendor/bin/php-cs-fixer fix --config=.php_cs.dist

[Edit] Maybe wait for the https://github.com/box/spout/pull/747 PR to be merged as it will fix most code style issues unrelated to your changes

jmsche avatar May 29 '20 09:05 jmsche

@jmsche it seems that I'll wait, as my spare time this month is pretty much zero :)

quamis avatar Jun 11 '20 08:06 quamis

添加了对 mergeCells的支持: // mergeCells(B2:G2),您可以使用CellHelper :: getColumnLettersFromColumnIndex()从“ B2”转换为“ [1,2]” $ writer-> mergeCells([1,2],[ 6,2]);

cell height:
    $row->setHeight(30);

shouldShrinkToFit:
    $style->setShouldShrinkToFit();

这些更改是针对XLSX实施的,因为这正是我所需要的,并且对测试喷口进行了测试。

How to write the code, I also need to merge the cells

TheSaltwaterRoom avatar Jun 13 '20 09:06 TheSaltwaterRoom

Hey guys and thanks for your work @quamis! This feature would be awesome for us. Can I do anything to make it happen?

ignaczistvan avatar Jul 31 '20 08:07 ignaczistvan

Hey guys and thanks for your work @quamis! This feature would be awesome for us. Can I do anything to make it happen?

You could provide the missing Unit Tests!

madflow avatar Aug 02 '20 18:08 madflow

Hey guys and thanks for your work @quamis! This feature would be awesome for us. Can I do anything to make it happen?

You could provide the missing Unit Tests!

@madflow Better late than never: https://github.com/quamis/spout/pull/1 Let me know if I could help with docs or anything else.

ignaczistvan avatar Aug 14 '20 16:08 ignaczistvan

@jmsche any ideea why "continuous-integration/travis-ci Expected — Waiting for status to be reported " hasn't finished yet? I made the commit 2 weeks ago

@ignaczistvan I do not understand. Did you made the unitTests (I'm not seeing anything, but again, I'm not familiar with github's way of mergeing stuff)? Or you would prefer helping with the docs?

quamis avatar Aug 14 '20 20:08 quamis

@ignaczistvan I do not understand. Did you made the unitTests (I'm not seeing anything, but again, I'm not familiar with github's way of mergeing stuff)? Or you would prefer helping with the docs?

@quamis Yep, I've made the tests and filed a pull request to your fork. You can check my commits here: https://github.com/quamis/spout/pull/1 If you like what you see, you can accept the PR. When you accept the PR, my changes will appear on this 'main' PR too.

I'm also open to helping with the docs if it's needed to release a new version.

ignaczistvan avatar Aug 14 '20 21:08 ignaczistvan

@madflow Any update on this one?

ignaczistvan avatar Aug 31 '20 07:08 ignaczistvan

@ignaczistvan Not sure what you mean... Are you referring to my hint above, that unit tests where missing?

madflow avatar Aug 31 '20 09:08 madflow

@ignaczistvan Not sure what you mean... Are you referring to my hint above, that unit tests where missing?

Well, I’ve added tests for the new features so basically yes. Is anything missing, something I missed on the tests, docs, etc?

ignaczistvan avatar Aug 31 '20 16:08 ignaczistvan

@ignaczistvan Not sure what you mean... Are you referring to my hint above, that unit tests where missing?

Well, I’ve added tests for the new features so basically yes. Is anything missing, something I missed on the tests, docs, etc?

You asked what you can do to bring this PR forward. After quick glance I noticed the missing unit tests.

From my experience: In order to raise chances that a maintainer will consider this PR "mergable" it should

  • [ ] include tests
  • [ ] include documentation
  • [ ] focus on a single feature
  • [ ] have feature parity in CSV/ODS/XLSX

From what I gather - the chances that a maintainer will have a look and give feedback are slim at the moment.

madflow avatar Aug 31 '20 18:08 madflow

This will add the merged cells on all sheets, not just the current active sheet.

eigan avatar Jan 31 '22 08:01 eigan

Merged in https://github.com/openspout/openspout/pull/21

Slamdunk avatar Mar 10 '22 08:03 Slamdunk