Skip to content
This repository has been archived by the owner on Dec 31, 2021. It is now read-only.

support for WithColumnFormatting Concern #8

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tomasvanrijsse
Copy link

@tomasvanrijsse tomasvanrijsse commented Sep 17, 2021

Requirements

Please take note of our contributing guidelines: https://nikazooz.github.io/laravel-simplesheet/1.0/getting-started/contributing
Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.

Mark the following tasks as done:

  • Checked the codebase to ensure that your feature doesn't already exist.
  • Checked the pull requests to ensure that another person hasn't already submitted the feature or fix.
  • Adjusted the Documentation.
  • Added tests to ensure against regression.

Description of the Change

I was migrating from Laravel/Excel to this package and I was missing the ability to format my columns.
https://docs.laravel-excel.com/3.1/exports/column-formatting.html#formatting-columns

Why Should This Be Added?

Because you try to be a drop-in replacement for Laravel/Excel

Benefits

The ability to format your columns

Possible Drawbacks

Unknown

Verification Process

My existing exports still worked 👍
I've also added tests, but I wasn't able to get them to run locally.

Applicable Issues

The formatting for date format don't work yet.
For those to work we need something like \PhpOffice\PhpSpreadsheet\Shared\Date::dateTimeToExcel() in the mapping to ensure correct parsing of dates.

@nikazooz
Copy link
Owner

nikazooz commented Sep 19, 2021

To run tests locally you most likely need to manually require laravel/legacy-factories composer package. Note that you shouldn't commit the changed composer.json in that case.

There seems to be some issue and the test you added is failing. You can check one of the failing workflows in this PR to see what's going on.

@tomasvanrijsse
Copy link
Author

I'll fix the test.
Although it might be in a different approach because it seems that box/sprout doesn't retrieve the styles it generated itself.
I'm thinking about just testing if Cell::setStyle() is being called with the expected arguments.

Apart from the tests, does this look like a feature you'd merge?

@nikazooz
Copy link
Owner

Happy to merge it if everything works as expected and the tests pass 🙂

@tomasvanrijsse
Copy link
Author

tomasvanrijsse commented Sep 21, 2021

The current test tries to set the Cell->style->format and this does show when you open the file.
The Box/Sprout reader however doesn't read the format :(

So I tried another test approach, just testing that the style is being set on the Cell and assuming that Box/Spout Writer will do its job.
Ideally I'd like to generate the sheet and check if the Box/Spout Writer has Cells with the desired format, the writer however doesn't expose the Cells anymore.
So I tried to something else; mocking the Cell setStyle or getStyle and see if it is being called.
To do so I had to make a small change to Sheet.php from new Cell() to app(Cell::class) to make that work.
This approach does give the desired assertions however is also breaks the Exporter->store method, and generates an Exception.

$expectedObject = (new StyleBuilder())
    ->setFormat(NumberFormat::FORMAT_NUMBER_00)
    ->build();

$cell = $this->mock(Cell::class)->makePartial()
    ->shouldReceive('setStyle')
    ->withArgs(function ($argument) use ($expectedObject) {
       /** @var Style $argument */
        return $argument->getFormat() === $expectedObject->getFormat();
    })
    ->andReturnSelf();

/** @var Exporter $export */
$export = app(WithColumnFormattingExport::class);

$response = $export->store('with-column-formatting-store.xlsx');

I'm a bit lost now on what approach to pursue. The first approach seems most like the the other tests but at the moment it seems to be blocked by an issue in the reader.

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.

2 participants