Conversation
src/CliMenu.php
Outdated
| $frame = new Frame; | ||
|
|
||
| $frame->newLine(2); | ||
|
|
There was a problem hiding this comment.
Maybe the $borderRow could be stored in the style so that it doesn't need to be recomputed each time but just when one of the values change, I'm not sure.
There was a problem hiding this comment.
Doesn't sound like a crazy idea
|
Looks good to me - like you mentioned privately, requires some updates for the builder and PSR2 fixes. Also would be nice for some tests and this breaks a lot of the existing tests as well. I can help out on Monday if necessary. |
Codecov Report
@@ Coverage Diff @@
## master #100 +/- ##
============================================
- Coverage 96.32% 96.18% -0.15%
- Complexity 307 337 +30
============================================
Files 23 23
Lines 926 1049 +123
============================================
+ Hits 892 1009 +117
- Misses 34 40 +6
Continue to review full report at Codecov.
|
| return $this; | ||
| } | ||
|
|
||
| public function setBorder( |
There was a problem hiding this comment.
I think we're missing the MenuStyle configuring which should be done in buildStyle()
| { | ||
| return $this->borderColour; | ||
| } | ||
| public function getBorderColourCode() : string |
There was a problem hiding this comment.
Please add a new line between each of these methods
|
Ok so the only that's missing are tests of actually outputing a menu with borders (that will hit those missing lines in codecov). I juste really didn't know where to do that (didn't find any tests that check that margin and paddings have an real effect on output, otherwise I would have pu that there). I may still make some adjustments to the code for optimization so don't merge it right away. The next step would be to add borders to dialogues I guess. |
|
This is looking good to me. I can add the tests after if you would like? Also we can do the border for inputs/dialogues as another PR, nicer to keep these features small and easier to review :) Let me know when your done and we get this merged. |
|
I'll try to pre generate the top and bottom borders instead of doing it each time the frame is updated before pushing. |
|
Closing/Reopening to trigger travis. |
| $this->style['borderBottomWidth'] = $bottomWidth; | ||
| $this->style['borderLeftWidth'] = $leftWidth; | ||
|
|
||
| if (is_string($colour)) { |
There was a problem hiding this comment.
Should we throw an exception if colour is not null and not a string? validateColour will check the colour later but not if someone does something weird like pass an object to it.
| /** | ||
| * @var string | ||
| */ | ||
| private $borderColour = 'white'; |
There was a problem hiding this comment.
Can probably drop this assignment.
There was a problem hiding this comment.
If I drop it I get an error in generateBorderRows when the border colour isn't set.
Juste like the issue you had with bg and fg colours earlier.
There was a problem hiding this comment.
Hmm - It works fine for me as private $borderColour. It is set in the constructor from $defaultStyleValues
src/MenuStyle.php
Outdated
| str_repeat(' ', $this->margin) | ||
| ); | ||
|
|
||
| $this->borderTopRows = []; |
There was a problem hiding this comment.
You could do something like the following if you wanted:
$this->borderTopRows = array_fill(0, $this->borderTopWidth, $borderRow)Your call, I don't mind :)
There was a problem hiding this comment.
Yep I'll do that good call
src/MenuStyle.php
Outdated
|
|
||
| public function getBorderColourCode() : string | ||
| { | ||
| if (ctype_digit($this->borderColour)) { |
There was a problem hiding this comment.
I think this if statement needs to be negated
|
Thank you @Lynesth - this is awesome! |
3 participants
No description provided.