Checkbox + Radio item-level styling #205

AydinHassan · 2019-12-19T20:33:37Z

#204 without checkbox rename

Related to #203 and #200

Renames CheckableItem to CheckboxItem
Adds item-level styling (markers, item extra and display extra)
Item styles are passed down from parent menu, or use default values

This does not fix the nested-style issue from #201

codecov-io · 2019-12-19T20:37:52Z

Codecov Report

Merging #205 into master will decrease coverage by 1.43%.
The diff coverage is 82.72%.

@@             Coverage Diff              @@
##             master     #205      +/-   ##
============================================
- Coverage     93.91%   92.48%   -1.44%     
- Complexity      516      559      +43     
============================================
  Files            29       30       +1     
  Lines          1561     1689     +128     
============================================
+ Hits           1466     1562      +96     
- Misses           95      127      +32

Impacted Files	Coverage Δ	Complexity Δ
src/MenuStyle.php	`96.99% <ø> (-0.29%)`	`70 <0> (-8)`
src/MenuItem/RadioItem.php	`100% <100%> (ø)`	`21 <11> (+11)`	⬆️
src/CliMenu.php	`95.25% <100%> (+0.2%)`	`113 <4> (+4)`	⬆️
src/MenuItem/SplitItem.php	`100% <100%> (ø)`	`45 <0> (-2)`	⬇️
src/Builder/CliMenuBuilder.php	`78.76% <50%> (-2.97%)`	`76 <6> (+6)`
src/Builder/SplitItemBuilder.php	`70.31% <61.53%> (-7.19%)`	`20 <6> (+8)`
src/MenuItem/CheckboxItem.php	`92.72% <87.87%> (-7.28%)`	`20 <11> (+11)`
src/Style/CheckboxStyle.php	`88.57% <88.57%> (ø)`	`12 <12> (?)`
src/Style/RadioStyle.php	`88.57% <88.57%> (ø)`	`12 <12> (?)`
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c72b588...4ea4242. Read the comment docs.

src/MenuItem/ToggableTrait.php

AydinHassan · 2019-12-19T20:41:29Z

src/Style/CheckboxStyle.php

+
+    public function __construct()
+    {
+        $this->setMarkerOn(self::DEFAULT_STYLES['markerOn']);


Doesn't this automatically set it as custom?

Yes, but then it has $this->custom = false;

src/Style/ItemStyleTrait.php

src/Builder/SplitItemBuilder.php

AydinHassan · 2019-12-19T20:48:57Z

src/Builder/SplitItemBuilder.php

+        $menu->radioStyle(function (RadioStyle $style) {
+            $style->fromArray($this->menu->getRadioStyle()->toArray());
+        });


Same as above

src/CliMenu.php

src/Style/CheckboxStyle.php

src/Builder/CliMenuBuilder.php

AydinHassan · 2019-12-19T21:21:06Z

src/Builder/CliMenuBuilder.php

+
+    public function radioStyle(callable $itemCallable) : self
+    {
+        $this->menu->radioStyle($itemCallable);


$itemCallable($this->menu->getRadioStyle())

AydinHassan · 2019-12-19T21:21:56Z

src/Builder/SplitItemBuilder.php

@@ -139,4 +168,18 @@ public function build() : SplitItem
    {
        return $this->splitItem;
    }
+
+    public function checkboxStyle(callable $itemCallable) : self


I don't quite understand this - Isn't this modifying the parent menu checkbox style?

It's for changing the style for items defined within an addSplitItem() call:

$menu = (new CliMenuBuilder) ->setWidth(150) ->addSplitItem(function (SplitItemBuilder $b) use ($itemCallable) { $b->addSubMenu('Sub Menu on a split item', function (CliMenuBuilder $b) { $b->setTitle('Behold the awesomeness') ->addItem('This is awesome', function() { print 'Yes!'; }) ->addSplitItem(function (SplitItemBuilder $b) { $b ->checkboxStyle(function (CheckboxStyle $style) { $style->setMarkerOn('[x]'); }) ->addItem('Split Item 1', function() { print 'Item 1!'; }) ->addItem('Split Item 2', function() { print 'Item 2!'; }) ->addItem('Split Item 3', function() { print 'Item 3!'; }) ->addSubMenu('Split Item Nested Sub Menu', function (CliMenuBuilder $b) { $b->addItem('One', function() { print 'One!'; }) ->addItem('Two', function() { print 'Two!'; }) ->addItem('Three', function() { print 'Three!'; }); }); }); }) ->addItem('Item 2', $itemCallable) ->addStaticItem('Item 3 - Static') ->addItem('Item 4', $itemCallable); }) ->build();

CliMenuBuilder::addSplitItem()'s style objects are separate from SplitItemBuilder's

hmm, it doesn't seem to work for me

<?php use PhpSchool\CliMenu\Builder\SplitItemBuilder; use PhpSchool\CliMenu\CliMenu; use PhpSchool\CliMenu\Builder\CliMenuBuilder; use PhpSchool\CliMenu\Style\CheckboxStyle; require_once(__DIR__ . '/../vendor/autoload.php'); $itemCallable = function (CliMenu $menu) { echo $menu->getSelectedItem()->getText(); }; $menu = (new CliMenuBuilder) ->setWidth(150) ->addSplitItem(function (SplitItemBuilder $b) use ($itemCallable) { $b->addSubMenu('Sub Menu on a split item', function (CliMenuBuilder $b) { $b->setTitle('Behold the awesomeness') ->addItem('This is awesome', function() { print 'Yes!'; }) ->addSplitItem(function (SplitItemBuilder $b) { $b ->modifyCheckboxStyle(function (CheckboxStyle $style) { $style->setMarkerOn('[x] '); }) ->addCheckboxItem('Rust', function() { print 'Item 1!'; }) ->addCheckboxItem('C++', function() { print 'Item 1!'; }) ->addCheckboxItem('Go', function() { print 'Item 1!'; }) ->addItem('Split Item 1', function() { print 'Item 1!'; }) ->addItem('Split Item 2', function() { print 'Item 2!'; }) ->addItem('Split Item 3', function() { print 'Item 3!'; }) ->addSubMenu('Split Item Nested Sub Menu', function (CliMenuBuilder $b) { $b->addItem('One', function() { print 'One!'; }) ->addItem('Two', function() { print 'Two!'; }) ->addItem('Three', function() { print 'Three!'; }); }); }); }) ->addItem('Item 2', $itemCallable) ->addStaticItem('Item 3 - Static') ->addItem('Item 4', $itemCallable); }) ->build(); $menu->open();

Checkbox + Radio item-level styling - CR Changes

README.md

src/Builder/CliMenuBuilder.php

Updates README radioStyle -> modifyRadioStyle

AydinHassan · 2019-12-19T21:58:30Z

src/Builder/CliMenuBuilder.php

+        if (!$menu->getCheckboxStyle()->hasChangedFromDefaults()) {
+            $menu->setCheckboxStyle(clone $this->menu->getCheckboxStyle());
+        }
+
+        if (!$menu->getRadioStyle()->hasChangedFromDefaults()) {
+            $menu->setRadioStyle(clone $this->menu->getRadioStyle());
+        }


We could extract this to a private method, since we do it below as well. But we can do that later.

Above even.

AydinHassan · 2019-12-19T22:04:37Z

src/Builder/SplitItemBuilder.php

    public function __construct(CliMenu $menu)
    {
        $this->menu = $menu;
        $this->splitItem = new SplitItem();
+
+        $this->checkboxStyle  = new CheckboxStyle();


So this seems to be inconsistent with the way CliMenu and CliMenuBuilder work. CliMenu holds the style instances, not the builder. The builder proxies to them. But here the SplitItemBuilder contains the style instances. Can we make it similar to the CliMenuBuilder works?

AydinHassan · 2019-12-19T22:16:29Z

src/Builder/SplitItemBuilder.php

+        return $this;
+    }
+
+    public function modifyCheckboxStyle(callable $itemCallable) : self


I actually think we should drop this. It's took me a while but I think I get what you are doing. If you change the styles in a splititem builder, they persist back to the parent menu. I think it makes the code very confusing. I would rather we just not allow to modify the styles in the SplitItem. Or rather if you modify the styles, it only applies to the split item and not the parent menu.

Style is always limited only to the current scope and any children:

<?php use PhpSchool\CliMenu\Builder\SplitItemBuilder; use PhpSchool\CliMenu\CliMenu; use PhpSchool\CliMenu\Builder\CliMenuBuilder; use PhpSchool\CliMenu\Style\CheckboxStyle; require_once(__DIR__ . '/../vendor/autoload.php'); $itemCallable = function (CliMenu $menu) { echo $menu->getSelectedItem()->getText(); }; $menu = (new CliMenuBuilder) ->setTitle('Select a Language') ->addCheckboxItem('Rust', $itemCallable) ->addCheckboxItem('C++', $itemCallable) ->addCheckboxItem('Go', $itemCallable) ->addCheckboxItem('Java', $itemCallable) ->addCheckboxItem('C', $itemCallable) ->addSplitItem(function (SplitItemBuilder $b) use ($itemCallable) { $b->setGutter(5) ->modifyCheckboxStyle(function (CheckboxStyle $style) { $style->setMarkerOff('ooo') ->setMarkerOn('xxx'); }) ->addCheckboxItem('Rust', $itemCallable) ->addCheckboxItem('C++', $itemCallable) ->addCheckboxItem('Go', $itemCallable) ->addCheckboxItem('Java', $itemCallable) ->addCheckboxItem('C', $itemCallable) ; }) ->build(); $menu->open();

The parent should never be affected from the modify*Style() methods:

Actually you're right, there's a bug where changing in a child updates the parent:

<?php use PhpSchool\CliMenu\Builder\SplitItemBuilder; use PhpSchool\CliMenu\CliMenu; use PhpSchool\CliMenu\Builder\CliMenuBuilder; use PhpSchool\CliMenu\Style\CheckboxStyle; require_once(__DIR__ . '/../vendor/autoload.php'); $itemCallable = function (CliMenu $menu) { echo $menu->getSelectedItem()->getText(); }; $menu = (new CliMenuBuilder) ->setTitle('Select a Language') ->addCheckboxItem('1-A', $itemCallable) ->addCheckboxItem('1-B', $itemCallable) ->addSplitItem(function (SplitItemBuilder $b) use ($itemCallable) { $b->setGutter(5) ->addCheckboxItem('2-A', $itemCallable) ->modifyCheckboxStyle(function (CheckboxStyle $style) { $style->setMarkerOff('o ') ->setMarkerOn('x '); }) ->addCheckboxItem('2-B', $itemCallable) ->addSubMenu('Options', function (CliMenuBuilder $b) use ($itemCallable) { $b->setTitle('CLI Menu > Options') ->addCheckboxItem('Rust', $itemCallable) ->addCheckboxItem('C++', $itemCallable) ->addCheckboxItem('Go', $itemCallable) ->addCheckboxItem('Java', $itemCallable) ->addCheckboxItem('C', $itemCallable) ->addLineBreak('-'); }) ->addCheckboxItem('2-C', $itemCallable) ->addCheckboxItem('2-D', $itemCallable) ->addCheckboxItem('2-E', $itemCallable) ; }) ->addCheckboxItem('1-C', $itemCallable) ->addCheckboxItem('1-D', $itemCallable) ->addCheckboxItem('1-E', $itemCallable) ->build(); $menu->open();

I'll fix and add a test.

Phew 😅 Thought I was going crazy

Right, I see, because SplitMenuBuilder::$menu is the exact same instance as CliMenuBuilder::$menu.

This is why SplitMenuBuilder had its own style instances (removed in #208).

Actually, I think the problem is that I had a fundamental misunderstanding on the SplitItem.

I was thinking this was a separate menu ala submenu, but it's simply a horizontal partitioner of the current menu.

You're completely right, SplitItemBuilder shouldn't have any of the radio/checkbox style methods, and should not have its own style instance(s).

SplitItemBuilder should reference menu styles

jtreminio · 2019-12-20T06:26:24Z

Please review and merge #210 before this PR.

It is going to be required for this process to work properly, and since there have been a ton of changes and I messed up on the SplitItem styling (#205 (comment)) it's probably best if I create a new PR without all the changes.

AydinHassan · 2019-12-20T07:07:30Z

@jtreminio #210 is merged now. Thank you. I think these changes should be a lot simpler now.

jtreminio · 2019-12-20T07:19:56Z

Competing PR at #211

jtreminio added 3 commits December 19, 2019 21:31

Item styling for Checkbox and Radio items

e58bb70

Updates README

34ea4b3

Removes extra code

6b72992

AydinHassan mentioned this pull request Dec 19, 2019

Checkbox + Radio item-level styling #204

Closed

jtreminio mentioned this pull request Dec 19, 2019

Item-specific styling #200

Closed

AydinHassan commented Dec 19, 2019

View reviewed changes

Drop comments

0c48311

AydinHassan commented Dec 19, 2019

View reviewed changes

src/Builder/CliMenuBuilder.php Outdated Show resolved Hide resolved

AydinHassan commented Dec 19, 2019

View reviewed changes

src/Builder/CliMenuBuilder.php Outdated Show resolved Hide resolved

AydinHassan commented Dec 19, 2019

View reviewed changes

src/Builder/CliMenuBuilder.php Outdated Show resolved Hide resolved

AydinHassan commented Dec 19, 2019

View reviewed changes

CR changes

8179b35

jtreminio mentioned this pull request Dec 19, 2019

Checkbox + Radio item-level styling - CR Changes #206

Merged

AydinHassan and others added 2 commits December 19, 2019 22:46

Merge pull request #206 from jtreminio/feature/item-style-2

d3a76ba

Checkbox + Radio item-level styling - CR Changes

Updates README radioStyle -> modifyRadioStyle

7a8a963

jtreminio mentioned this pull request Dec 19, 2019

Updates README radioStyle -> modifyRadioStyle #207

Merged

AydinHassan commented Dec 19, 2019

View reviewed changes

README.md Outdated Show resolved Hide resolved

AydinHassan commented Dec 19, 2019

View reviewed changes

README.md Outdated Show resolved Hide resolved

AydinHassan commented Dec 19, 2019

View reviewed changes

src/Builder/CliMenuBuilder.php Show resolved Hide resolved

Merge pull request #207 from jtreminio/feature/item-style-2

da43609

Updates README radioStyle -> modifyRadioStyle

AydinHassan commented Dec 19, 2019

View reviewed changes

SplitItemBuilder should reference menu styles

0f1db5f

jtreminio mentioned this pull request Dec 19, 2019

SplitItemBuilder should reference menu styles #208

Merged

Merge pull request #208 from jtreminio/feature/item-style-2

4ea4242

SplitItemBuilder should reference menu styles

jtreminio mentioned this pull request Dec 20, 2019

Removes styles from SplitItem #209

Closed

AydinHassan closed this Dec 20, 2019

AydinHassan deleted the item-style branch December 20, 2019 07:28

AydinHassan restored the item-style branch February 15, 2020 12:08

AydinHassan deleted the item-style branch February 15, 2020 19:01

Checkbox + Radio item-level styling #205

Checkbox + Radio item-level styling #205

Uh oh!

Conversation

AydinHassan commented Dec 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Dec 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AydinHassan Dec 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtreminio commented Dec 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AydinHassan commented Dec 20, 2019

Uh oh!

jtreminio commented Dec 20, 2019

Uh oh!

Uh oh!

AydinHassan commented Dec 19, 2019 •

edited

Loading

codecov-io commented Dec 19, 2019 •

edited

Loading

AydinHassan Dec 19, 2019 •

edited

Loading

jtreminio commented Dec 20, 2019 •

edited

Loading