Skip to content
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

Chart: Correctly handle axis line styles #3163

Merged
merged 11 commits into from
Nov 17, 2022
Merged

Chart: Correctly handle axis line styles #3163

merged 11 commits into from
Nov 17, 2022

Conversation

fox34
Copy link
Contributor

@fox34 fox34 commented Nov 10, 2022

Bugfix: Write line styles for y-axis of charts, not only x-axis, and read line styles for both axis.

This is:

- [X] a bugfix
- [ ] a new feature
- [ ] refactoring
- [ ] additional unit tests

Checklist:

Why this change is needed?

Line styles for y-axis could previously be set, but would not be saved to Xlsx files. Line styles for both x and y-axis were not read from Xlsx files.

Bugfix: Also write line styles for y-axis of charts
@fox34 fox34 changed the title Chart write y axis line styles XlsxWriter: Preserve y-axis line styles Nov 10, 2022
@oleibman
Copy link
Collaborator

Can you add a unit test, or update a sample, to demonstrate that this situation was not handled correctly before and is handled correctly now?

@fox34
Copy link
Contributor Author

fox34 commented Nov 12, 2022

Of course. Based on /samples/templates/chartSpreadsheet.php:

<?php

use PhpOffice\PhpSpreadsheet\Chart\Axis;
use PhpOffice\PhpSpreadsheet\Chart\Chart;
use PhpOffice\PhpSpreadsheet\Chart\DataSeries;
use PhpOffice\PhpSpreadsheet\Chart\DataSeriesValues;
use PhpOffice\PhpSpreadsheet\Chart\Legend as ChartLegend;
use PhpOffice\PhpSpreadsheet\Chart\PlotArea;
use PhpOffice\PhpSpreadsheet\Chart\Title;
use PhpOffice\PhpSpreadsheet\Spreadsheet;
use PhpOffice\PhpSpreadsheet\Writer\Xlsx as XlsxWriter;

// [...] Sample code from chartSpreadsheet.php omitted

// Create x- and y-axis
$xAxis = new Axis();
$xAxis->setLineColorProperties('FF0000');

$yAxis = new Axis();
$yAxis->setLineColorProperties('FF0000');

// Create the chart
$chart = new Chart(
    'chart1', // name
    $title, // title
    $legend, // legend
    $plotArea, // plotArea
    true, // plotVisibleOnly
    DataSeries::EMPTY_AS_GAP, // displayBlanksAs
    null, // xAxisLabel
    $yAxisLabel,  // yAxisLabel
    $xAxis,  // xAxis
    $yAxis   // yAxis
);

// Set the position where the chart should appear in the worksheet
$chart->setTopLeftPosition('A7');
$chart->setBottomRightPosition('H20');

// Add the chart to the worksheet
$worksheet->addChart($chart);

$writer = new XlsxWriter($spreadsheet);
$writer->setIncludeCharts(true);
$writer->save('out.xlsx');

Result before applying my patch:
grafik

Result after applying my patch:
grafik

@oleibman
Copy link
Collaborator

Please add that test/sample to your PR.

@fox34
Copy link
Contributor Author

fox34 commented Nov 14, 2022

Seems like the reader does not read line styles correctly, either; neither for x, nor for y-axis. The resulting XLSX definitely contains the correct line styles.

@fox34 fox34 changed the title XlsxWriter: Preserve y-axis line styles Chart: Correctly handle axis line styles Nov 14, 2022
@fox34
Copy link
Contributor Author

fox34 commented Nov 14, 2022

Fixed in 18714e3.

@oleibman
Copy link
Collaborator

Thank you, that was exactly what I had in mind. Now I need to get a little nitpicky, and we should be ready to roll after that. Can you please set your axes to different colors in your test (e.g. set x-axis to 00FF00) so that the test shows that the correct style is applied to the correct axis.

@oleibman
Copy link
Collaborator

@MarkBaker @PowerKiKi This PR is ready but ... It still thinks the PHP 7.3 test is required, and so the "Squash and merge" button is grayed out for me. As a result, I don't know how to merge it.

@MarkBaker MarkBaker merged commit 26cb1c1 into PHPOffice:master Nov 17, 2022
@fox34 fox34 deleted the chart-write-y-axis-line-styles branch November 17, 2022 09:50
@oleibman
Copy link
Collaborator

Thank you for your contribution.

MarkBaker added a commit that referenced this pull request Dec 21, 2022
### Added

- Extended flag options for the Reader `load()` and Writer `save()` methods
- Apply Row/Column limits (1048576 and XFD) in ReferenceHelper [PR #3213](#3213)
- Allow the creation of In-Memory Drawings from a string of binary image data, or from a stream. [PR #3157](#3157)
- Xlsx Reader support for Pivot Tables [PR #2829](#2829)
- Permit Date/Time Entered on Spreadsheet to be calculated as Float [Issue #1416](#1416) [PR #3121](#3121)

### Changed

- Nothing

### Deprecated

- Direct update of Calculation::suppressFormulaErrors is replaced with setter.
- Font public static variable defaultColumnWidths replaced with constant DEFAULT_COLUMN_WIDTHS.
- ExcelError public static variable errorCodes replaced with constant ERROR_CODES.
- NumberFormat constant FORMAT_DATE_YYYYMMDD2 replaced with existing identical FORMAT_DATE_YYYYMMDD.

### Removed

- Nothing

### Fixed

- Fixed handling for `_xlws` prefixed functions from Office365 [Issue #3245](#3245) [PR #3247](#3247)
- Conditionals formatting rules applied to rows/columns are removed [Issue #3184](#3184) [PR #3213](#3213)
- Treat strings containing currency or accounting values as floats in Calculation Engine operations [Issue #3165](#3165) [PR #3189](#3189)
- Treat strings containing percentage values as floats in Calculation Engine operations [Issue #3155](#3155) [PR #3156](#3156) and [PR #3164](#3164)
- Xlsx Reader Accept Palette of Fewer than 64 Colors [Issue #3093](#3093) [PR #3096](#3096)
- Use Locale-Independent Float Conversion for Xlsx Writer Custom Property [Issue #3095](#3095) [PR #3099](#3099)
- Allow setting AutoFilter range on a single cell or row [Issue #3102](#3102) [PR #3111](#3111)
- Xlsx Reader External Data Validations Flag Missing [Issue #2677](#2677) [PR #3078](#3078)
- Reduces extra memory usage on `__destruct()` calls [PR #3092](#3092)
- Additional properties for Trendlines [Issue #3011](#3011) [PR #3028](#3028)
- Calculation suppressFormulaErrors fix [Issue #1531](#1531) [PR #3092](#3092)
- Permit Date/Time Entered on Spreadsheet to be Calculated as Float [Issue #1416](#1416) [PR #3121](#3121)
- Incorrect Handling of Data Validation Formula Containing Ampersand [Issue #3145](#3145) [PR #3146](#3146)
- Xlsx Namespace Handling of Drawings, RowAndColumnAttributes, MergeCells [Issue #3138](#3138) [PR #3136](#3137)
- Generation3 Copy With Image in Footer [Issue #3126](#3126) [PR #3140](#3140)
- MATCH Function Problems with Int/Float Compare and Wildcards [Issue #3141](#3141) [PR #3142](#3142)
- Fix ODS Read Filter on number-columns-repeated cell [Issue #3148](#3148) [PR #3149](#3149)
- Problems Formatting Very Small and Very Large Numbers [Issue #3128](#3128) [PR #3152](#3152)
- XlsxWrite preserve line styles for y-axis, not just x-axis [PR #3163](#3163)
- Xlsx Namespace Handling of Drawings, RowAndColumnAttributes, MergeCells [Issue #3138](#3138) [PR #3137](#3137)
- More Detail for Cyclic Error Messages [Issue #3169](#3169) [PR #3170](#3170)
- Improved Documentation for Deprecations - many PRs [Issue #3162](#3162)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants