Checkbox + Radio item-level styling (3rd try)#211
Checkbox + Radio item-level styling (3rd try)#211AydinHassan merged 2 commits intophp-school:masterfrom jtreminio:feature/checkbox-radio-style
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.
|
| $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.
| $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.
3 participants
Related to #203 and #200