Graph custom colors#768
Conversation
PowerKiKi
left a comment
PowerKiKi
left a comment
There was a problem hiding this comment.
It looks like an interesting new feature, but there are a few things that need to be improved before merging.
Also make sure to review the PR checklist (missing changelog).
| try { | ||
| $testInstance->setFillColor('WRONG COLOR'); | ||
| } catch (Exception $e) { | ||
| self::assertEquals($e->getMessage(), 'Invalid hex color for chart series'); |
There was a problem hiding this comment.
This should use PHPunit's $this->expectExceptionMessage($expectMessage); in a separate test case.
| try { | ||
| $testInstance->setFillColor(['b8292f', 'WRONG COLOR']); | ||
| } catch (Exception $e) { | ||
| self::assertEquals($e->getMessage(), 'Invalid hex color for chart series (color: "WRONG COLOR")'); |
| * Fill color (can be array with colors if dataseries have custom colors). | ||
| * | ||
| * @var string | ||
| * @var array|string |
There was a problem hiding this comment.
This should be and all similar type hinting too:
| * @var array|string | |
| * @var string[]|string |
There was a problem hiding this comment.
I have changed it, however phpcs suggest string|string[] instead of string[]|string
| } | ||
| } | ||
| } else { | ||
| if (!preg_match('/^[a-f0-9]{6}$/i', $color)) { |
There was a problem hiding this comment.
Avoid duplicating validation code by creating a private method like "validateColor(string $color): void" that throw exception if need be.
| $fillColorValues = $plotSeriesValues->getFillColor(); | ||
| if ($fillColorValues !== null && is_array($fillColorValues)) { | ||
| foreach ($plotSeriesValues->getDataValues() as $dataKey => $dataValue) { | ||
| $objWriter->startElement('c:dPt'); |
There was a problem hiding this comment.
This should be extract into private method to avoid duplicating code
| } | ||
|
|
||
| // Values | ||
| $plotSeriesValues = $plotGroup->getPlotValuesByIndex($plotSeriesRef); |
There was a problem hiding this comment.
It was moved to line 1100 ($plotSeriesValues is needed earlier).
…uszczynski/PhpSpreadsheet into graph_multiple_colors
|
@PowerKiKi I hope I have made all what is needed. |
|
Thanks, I rebased and merged your work. |
This is an follow-up for PHPOffice#158 Fixes PHPOffice#768
This is an follow-up for PHPOffice#158 Fixes PHPOffice#768
2 participants
This is:
Checklist:
Why this change is needed?
To customize colors on pie and donut charts.