support justifyLastLine #4373
support justifyLastLine #4373
Conversation
|
You will need to fix the Phpstan problems and the unit tests that broke as a result of your change. Then you need to make sure that the new property in Alignment is used when calculating the alignment hash code. Then you need to add some tests to demonstrate that your change is working - at a minimum, set the new property in some cell on a spreadsheet (and leave it unset in other cells), write out the spreadsheet, read it back in, and confirm that the property is set or not as desired. |
|
I'd fix the Phpstan problems and the unit tests. |
|
Good work on correcting the earlier problems. You still need to include the new field when generating the hash code for Alignment, and to add unit test(s). |
Excuse me, but which file should I add the test to? |
| @@ -265,6 +265,8 @@ public function readAlignmentStyle(Alignment $alignment, SimpleXMLElement $align | |||
| if ($horizontal !== '') { | |||
| $alignment->setHorizontal($horizontal); | |||
| } | |||
| $justifyLastLine = (string) $this->getAttribute($alignmentXml, 'justifyLastLine'); | |||
There was a problem hiding this comment.
This isn't quite right. Because justifyLastLine can be null as well as true/false, if justifyLastLine isn't in the Xml, you should leave it as its default property. Your code will set it to false. It probably doesn't make a difference to the end result, but we strive to be "correct".
| @@ -37,6 +37,7 @@ public function testNonDefaultAlign(): void | |||
| $rsheet = $reloadedSpreadsheet->getActiveSheet(); | |||
| $expected1 = [ | |||
| 'horizontal' => 'center', | |||
| 'justifyLastLine' => false, | |||
There was a problem hiding this comment.
This will probably be affected by the change I suggested above to Reader/Xlsx/Styles.
| @@ -78,6 +79,7 @@ public function testDefaultAlign(): void | |||
| $rsheet = $reloadedSpreadsheet->getActiveSheet(); | |||
| $expected1 = [ | |||
| 'horizontal' => 'general', | |||
| 'justifyLastLine' => false, | |||
There was a problem hiding this comment.
This will probably be affected by the change I suggested above to Reader/Xlsx/Styles.
| $sheet = $this->spreadsheet->getActiveSheet(); | ||
| $cell1 = $sheet->getCell('A1'); | ||
| $cell1->setValue('ABC'); | ||
| $cell1->getStyle()->getAlignment()->setHorizontal(Alignment::HORIZONTAL_DISTRIBUTED); |
There was a problem hiding this comment.
Your test is not doing enough (or you want to add an additional test in Reader/Xlsx). We want to write and then read the spreadsheet to prove that both Write and Read handle the new attribute correctly. Look for examples of this by searching for "writeAndReload" in the PhpSpreadsheetTests Reader/Xlsx directory.
| @@ -475,6 +512,7 @@ public function getHashCode(): string | |||
|
|
|||
| return md5( | |||
| $this->horizontal | |||
| . $this->justifyLastLine | |||
There was a problem hiding this comment.
Another case where the 3-pronged value will cause problems. Probably change this to:
. (($this->justifyLastLine === null) ? 'null' : ($this->justifyLastLine ? 't' : 'f'))
|
Despite the number of changes I've requested, you're in pretty good shape. I expect this will be ready to go soon. |
This is:
Checklist:
Why this change is needed?
justifyLastLine” is a flag used for the ‘Justify Distributed’ option, which is enabled only when the value of ‘Horizontal’ in the EXCEL setting ‘FormatCells’ is ‘Distributed’. When the option is enabled, an indent of a certain width is added before and after the text in the cell.
This option is often used in Japanese-speaking countries.