Skip to content
This repository has been archived by the owner on May 26, 2022. It is now read-only.

Added support for mergeCells, cell height, shrink to fit #738

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

quamis
Copy link

@quamis quamis commented May 4, 2020

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.

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
Copy link
Author

quamis commented May 4, 2020

Should close #529 (comment) & #729

@quamis
Copy link
Author

quamis commented May 4, 2020

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

@CLAassistant
Copy link

CLAassistant commented May 4, 2020

CLA assistant check
All committers have signed the CLA.

@jmsche
Copy link
Contributor

jmsche commented May 29, 2020

@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 #747 PR to be merged as it will fix most code style issues unrelated to your changes

@quamis
Copy link
Author

quamis commented Jun 11, 2020

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

@TheSaltwaterRoom
Copy link

添加了对
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

@ignaczistvan
Copy link

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

@madflow
Copy link
Contributor

madflow commented Aug 2, 2020

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!

shrinkToFit was not handled in StyleMerger so it was overwritten by the default cell style
StyleManager didn't add the property to the xml if shrinkToFit was used without alignment or text wrap.
Unit test
…efact

Tests
Added 'addOption' to OptionsManagerInterface
Moved 'setColumnWidths' and 'mergeCells' methods to Xlsx Writer implementation since the actual feature only works for Xlsx at the moment
@ignaczistvan
Copy link

ignaczistvan commented Aug 14, 2020

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: quamis#1
Let me know if I could help with docs or anything else.

@quamis
Copy link
Author

quamis commented Aug 14, 2020

@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?

@ignaczistvan
Copy link

ignaczistvan commented Aug 14, 2020

@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: quamis#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.

Test + minor fixes/refacts
@ignaczistvan
Copy link

@madflow Any update on this one?

@madflow
Copy link
Contributor

madflow commented Aug 31, 2020

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

@ignaczistvan
Copy link

@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?

@madflow
Copy link
Contributor

madflow commented Aug 31, 2020

@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.

@eigan
Copy link

eigan commented Jan 31, 2022

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

Slamdunk pushed a commit to Slamdunk/openspout that referenced this pull request Mar 10, 2022
@Slamdunk
Copy link

Merged in openspout/openspout#21

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants