Conditional and table formatting support for html #4412
Conversation
tests/PhpSpreadsheetTests/Shared/StringHelperInvalidCharTest.php
Outdated
Show resolved
Hide resolved
tests/PhpSpreadsheetTests/Writer/Html/HtmlConditionalFormattingTest.php
Outdated
Show resolved
Hide resolved
|
Here is something like what I would like to see for HtmlTableFormatWithConditionalTest. |
|
I did some profiling and found out that the getHashCode functions in Style/ get called a lot. They make the conditional formatting pretty slow. I managed to cache the hash code so that the execution time dropped by 75%. @oleibman Should I commit that to this PR (I think yes) or make another pull request after this one?
|
mikkokoo
force-pushed
the
master
branch
2 times, most recently
from
4272f4d to
d542036
Compare
|
Please make a separate PR for your HashCode improvements. |
| @@ -214,6 +215,15 @@ protected function processOperatorComparison(Conditional $conditional): bool | |||
| return $this->evaluateExpression($expression); | |||
| } | |||
|
|
|||
| protected function processColorScale(Conditional $conditional): bool | |||
| { | |||
| if ($this->wrapCellValue() !== '' && is_float($this->wrapCellValue()) && $conditional->getColorScale() !== null && $conditional->getColorScale()->colorScaleReadyForUse()) { | |||
There was a problem hiding this comment.
If I understand your code correctly your first 2 conditions conditions can be combined - if it's a float, then it's not equal to null-string. And I also believe the 3rd and 4th conditions can also be combined with a null-safe operator.
if (is_float($this->wrapCellValue()) && $conditional->getColorScale?->colorScaleReadyForUse())However, I'm not entirely sure about is_float. It seems that wrapCellValue might also return int. If you don't want that to follow the same code path as float, then leave is_float as-is; however, if you do want int and float to follow the same path, you might use is_numeric instead.
|
|
||
| public static function colourScaleProvider(): array | ||
| { | ||
| return [ |
There was a problem hiding this comment.
This is still an overwhelmingly large number of tests. Can you model it after the others and just test a few "interesting" cells in each row, and only for the data you're looking for (background color and value).
There was a problem hiding this comment.
It is the test of different color scale formulas. There has to be quite a few data points to have meaningful results as the style is the only measurable data. They are all interesting. I'd say this is the bare minimum.
|
|
||
| public function testConditionalFormattingRulesHtml(): void | ||
| { | ||
| $expectedMatches = [ |
There was a problem hiding this comment.
Again, this is just too many tests for me to absorb. Please just choose a representative sample. Maybe a couple from column 1 (all of this column seems to match whatever test it needs), the matching cell from column 2, one or two non-matching cells from column 2, and one or two cells from column 3 (these might be a little trickier because conditional formatting doesn't apply to them). It is not that the tests I'm asking you to remove aren't worthwhile - they most decidedly are. For that reason, I would suggest commenting them out rather than deleting them. I think the "missing" tests are better handled by also creating new samples, so that we can confirm visually that the html matches the original xlsx. I will add a comment to your ticket later on how that might be done.
|
As discussed above, I would like you to use your xlsx files for both unit tests and samples. Here is how you might go about it. |
This is:
Checklist:
Why this change is needed?
Conditional and table formatting support for html writer were needed to provide better html output for processed files.
Fix #1375.