Checkbox + Radio item-level styling (3rd try) #211
Conversation
Codecov Report
@@ Coverage Diff @@
## master #211 +/- ##
============================================
- Coverage 94% 93.04% -0.97%
- Complexity 521 558 +37
============================================
Files 29 30 +1
Lines 1569 1667 +98
============================================
+ Hits 1475 1551 +76
- Misses 94 116 +22
Continue to review full report at Codecov.
|
| @@ -560,6 +572,18 @@ private function propagateStyles(CliMenu $menu, array $items = []) | |||
| $currentItems = !empty($items) ? $items : $menu->getItems(); | |||
|
|
|||
| foreach ($currentItems as $item) { | |||
| if ($item instanceof CheckboxItem | |||
There was a problem hiding this comment.
The late style binding here really helps reduce complexity. As more item types are refactored to support item-level styling, they should be added here to make them work properly.
| if ($item instanceof CheckboxItem | ||
| && !$item->getStyle()->hasChangedFromDefaults() | ||
| ) { | ||
| $item->setStyle(clone $menu->getCheckboxStyle()); | ||
| } | ||
|
|
||
| if ($item instanceof RadioItem | ||
| && !$item->getStyle()->hasChangedFromDefaults() | ||
| ) { | ||
| $item->setStyle(clone $menu->getRadioStyle()); | ||
| } |
There was a problem hiding this comment.
I don't like this and suspect it will get uglier. But also I'm happy for us to fix this later.
There was a problem hiding this comment.
I mean refactor, just because of all the instanceof checks.
|
|
||
| class CheckboxStyle | ||
| { | ||
| protected const DEFAULT_STYLES = [ |
There was a problem hiding this comment.
I'd like to make all these private. I will do it after merge.
|
|
||
| namespace PhpSchool\CliMenu\Style; | ||
|
|
||
| class CheckboxStyle |
There was a problem hiding this comment.
Also I will add some simple tests for these Style objects.
| ->setUncheckedMarker('[○] ') | ||
| ->setCheckedMarker('[●] ') | ||
| ->modifyCheckboxStyle(function (CheckboxStyle $style) { | ||
| $style->setMarkerOff('[○] ') |
There was a problem hiding this comment.
I think I prefer setCheckedMarker and setUncheckedMarker. We can address that later.
There was a problem hiding this comment.
I was hoping to keep the naming scheme identical across Selectable, Checkbox and Radio items.
There was a problem hiding this comment.
However, since removed all the interfaces it doesn't matter much anymore.
There was a problem hiding this comment.
I think I'd prefer to keep it more semantic. And I think in the end, we might push this rendering back in to the item.
There was a problem hiding this comment.
I've got PR for SelectableItem styling ready. Should I hold off on it, or should I submit? The code will follow this PR.
| @@ -568,6 +592,14 @@ private function propagateStyles(CliMenu $menu, array $items = []) | |||
| $subMenu->setStyle(clone $menu->getStyle()); | |||
| } | |||
|
|
|||
| if (!$subMenu->getCheckboxStyle()->hasChangedFromDefaults()) { | |||
There was a problem hiding this comment.
Each submenu object gets a copy of radio and checkbox style object to keep. This is what is handed out to children items.
Related to #203 and #200