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

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

Open
quamis wants to merge 6 commits into
box:masterfrom
quamis:master
Open

Added support for mergeCells, cell height, shrink to fit#738
quamis wants to merge 6 commits into
box:masterfrom
quamis:master

Conversation

@quamis

@quamis quamis commented May 4, 2020

Copy link
Copy Markdown

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

quamis commented May 4, 2020

Copy link
Copy Markdown
Author

Should close #529 (comment) & #729

@quamis

quamis commented May 4, 2020

Copy link
Copy Markdown
Author

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

@CLAassistant

CLAassistant commented May 4, 2020

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@jmsche

jmsche commented May 29, 2020

Copy link
Copy Markdown
Contributor

@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

quamis commented Jun 11, 2020

Copy link
Copy Markdown
Author

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

@TheSaltwaterRoom

Copy link
Copy Markdown

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

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

@madflow

madflow commented Aug 2, 2020

Copy link
Copy Markdown
Contributor

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

ignaczistvan commented Aug 14, 2020

Copy link
Copy Markdown

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

quamis commented Aug 14, 2020

Copy link
Copy Markdown
Author

@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

ignaczistvan commented Aug 14, 2020

Copy link
Copy Markdown

@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
Copy Markdown

@madflow Any update on this one?

@madflow

madflow commented Aug 31, 2020

Copy link
Copy Markdown
Contributor

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

@ignaczistvan

Copy link
Copy Markdown

@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

madflow commented Aug 31, 2020

Copy link
Copy Markdown
Contributor

@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

eigan commented Jan 31, 2022

Copy link
Copy Markdown

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
Copy Markdown

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.

8 participants