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

Added Conditional Formatting: ColorScale for Xlsx #3738

Merged
merged 8 commits into from
Sep 30, 2023

Conversation

redtailmatt
Copy link
Contributor

@redtailmatt redtailmatt commented Sep 15, 2023

This is:

  • a bugfix
  • a new feature
  • refactoring
  • additional unit tests

Checklist:

  • Changes are covered by unit tests
    • Changes are covered by existing unit tests
    • New unit tests have been added
  • Code style is respected
  • Commit message explains why the change is made (see https://github.com/erlang/otp/wiki/Writing-good-commit-messages)
  • CHANGELOG.md contains a short summary of the change and a link to the pull request if applicable
  • Documentation is updated as necessary

Why this change is needed?

ColorScale was not previously supported. Now it is. Currently, only the Xlsx Writer has been updated.

@oleibman
Copy link
Collaborator

Thank you for starting this; it seems like it could be a valuable addition. However, there are a lot of changes that will need to be made before it is ready.

You will need to eliminate the phpcs problems. I believe these are due to the use of tabs rather than spaces as whitespace in some of your statements.

You will need to eliminate the phpstan problems. I think many of those will be solved by adding

use PhpOffice\PhpSpreadsheet\Style\Color;

in ConditionalColorScale.php. Then, you need to adjust the types of your parameters and return values, e.g. setMinimumColor probably requires a Color parameter, not a string.

Once we have satisfied phpcs and phpstan, your code is still only half there. While you can make a case for adding it only to the Xlsx Writer, you can make a much stronger case if you add corresponding support to the Xlsx Reader as well.

Then, once that is taken care of, we will need to add some unit tests to demonstrate that your code is working correctly.

@oleibman
Copy link
Collaborator

No concerns with Scrutinizer "complexity" warning.

The implementation of DataBar looks for an rgb attribute, but the color may be provided via theme attribute instead. The initial implementation for ColorScale did the same. Change both to use the existing code in Reader\Xlsx\Styles to parse the color.
@oleibman oleibman merged commit a713e15 into PHPOffice:master Sep 30, 2023
11 checks passed
@oleibman
Copy link
Collaborator

Thank you for your contribution.

@redtailmatt redtailmatt deleted the ColorScale branch October 4, 2023 18:49
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.

2 participants