Skip to content

Allow setting fill colour for bar chart data points #2724

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Allow setting fill colour for bar chart data points #2724

wants to merge 2 commits into from

Conversation

DanWin
Copy link

@DanWin DanWin commented Apr 1, 2022

This is:

  • a bugfix
  • a new feature

Checklist:

Why this change is needed?

This will fix #1400 #1433 and #2183

It should be possible to set fill colours on bar charts just as it is aloready possible for donut charts and pie charts
@MarkBaker
Copy link
Member

MarkBaker commented Apr 2, 2022

Thank you for this contribution.

Is it possible to provide some unit tests to verify this for all chart types that are affected? Charts is one area that is the least covered by tests, and it really needs some TLC there.

@@ -1082,7 +1082,7 @@ private function writePlotGroup(?DataSeries $plotGroup, $groupType, XMLWriter $o
// Values
$plotSeriesValues = $plotGroup->getPlotValuesByIndex($plotSeriesRef);

if (($groupType == DataSeries::TYPE_PIECHART) || ($groupType == DataSeries::TYPE_PIECHART_3D) || ($groupType == DataSeries::TYPE_DONUTCHART)) {
if (in_array($groupType, [DataSeries::TYPE_PIECHART, DataSeries::TYPE_PIECHART_3D, DataSeries::TYPE_DONUTCHART, DataSeries::TYPE_BARCHART, DataSeries::TYPE_BARCHART_3D])) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in_array() should always be used in strict mode

Copy link
Member

@MarkBaker MarkBaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address requested change; and unit testing

@DanWin
Copy link
Author

DanWin commented Apr 3, 2022

Agreed, in_array() should be in strict mode. I saw it mostly being non-strict in other parts of the code and the previous checks were also non-strict and thus kept it the same way.
As for unit testing, I'll need help with that. I've never written those and I'm not sure how to write tests for this case.

@MarkBaker
Copy link
Member

Agreed, in_array() should be in strict mode. I saw it mostly being non-strict in other parts of the code and the previous checks were also non-strict and thus kept it the same way.

Thanks. A lot of the code that uses in_array() was originally written for earlier versions of PHP, before the strict mode flag was added. Now, we always try to apply it when we're working on those parts of the codebase where it isn't yet applied (scouting rules).

As for unit testing, I'll need help with that. I've never written those and I'm not sure how to write tests for this case.

There's some examples on how to approach testing in /tests/PhpSpreadsheetTests/Writer/Xlsx/* where we use the writer calls to return the XML to the test rather than actually saving to a file; and can then assert that the XML contains the markup that's expected.
e.g. ConditionalTest.php and the testWriteSimpleCellConditionalFromWizard() method.

Though you might want to create a Charts subfolder under .../Writer/Xlsx/, and put the test in there.

Something like:

namespace PhpOffice\PhpSpreadsheetTests\Writer\Xlsx\Charts;

use PhpOffice\PhpSpreadsheet\Spreadsheet;
use PhpOffice\PhpSpreadsheetTests\Functional\AbstractFunctional;

class BarChartTest extends AbstractFunctional
{
    public function testWriteBarChartWithUserDefinedFillColors(): void
    {
        $spreadsheet = new Spreadsheet();
        $worksheet = $spreadsheet->getActiveSheet();

        // Code to create a bar chart with user-defined fill colours goes here

        $writer = new Xlsx($spreadsheet);
        $writerWorksheet = new Xlsx\Worksheet($writer);
        $data = $writerWorksheet->writeWorksheet($worksheet, []);

        $expected = <<<XML
//xml for a bar chart with user-defined fill colours goes here
XML;
        self::assertStringContainsString($expected, $data);
    }
}

@MarkBaker MarkBaker added the writer/xlsx Writer for MS OfficeOpenXML-format (xlsx) spreadsheet files label Apr 23, 2022
oleibman added a commit to oleibman/PhpSpreadsheet that referenced this pull request Jun 7, 2022
Add a little more coverage, and use a neat trick suggested by @MarkBaker in the discussion of PR PHPOffice#2724 to greatly simplify MultiplierTest.
oleibman added a commit that referenced this pull request Jun 10, 2022
* Add Support to Chart/Axis and Gridlines for Shadow

Continuing the work from #2865. Support is added for Shadow properties for Axis and Gridlines, and Glow and SoftEdges are extended to Gridlines. Tests are added. Some chart tests are moved from Reader/Xlsx and Writer/Xlsx so that most chart tests are under a single directory.

This is a minor breaking change. Since the support for these properties was just added, it can't really affect much in userland. Some properties had been stored in the form which the XML requires them rather than as the user would enter them to Excel. So, for example, setting the Glow size to 10 points would have caused it to be stored internally as 127,000. This change will store the size internally as 10, obviously making the appropriate conversion when reading from or writing to XML. This makes unit tests much simpler, and I think this is also what a user would expect, especially considering the difficulties in keeping track of the trailing zeros.

* More Tests

Confirm value change between internal and xml.

* Still More Tests

Add a little more coverage, and use a neat trick suggested by @MarkBaker in the discussion of PR #2724 to greatly simplify MultiplierTest.
@oleibman oleibman marked this pull request as draft June 26, 2022 06:49
@oleibman
Copy link
Collaborator

This PR will be superseded by #2906, including tests. Converting this to draft; and will close it if 2906 is merged.

@oleibman
Copy link
Collaborator

Superseded by 2906, which was just merged.

@oleibman oleibman closed this Jun 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
charts writer/xlsx Writer for MS OfficeOpenXML-format (xlsx) spreadsheet files
Development

Successfully merging this pull request may close these issues.

Bar chart fillcolor not working
3 participants