Item-level style example; fixes disappearing styles for nested #203

jtreminio · 2019-12-18T21:29:41Z

Not ready for merge. Needs feedback, unit tests and general cleanup.

jtreminio · 2019-12-18T21:40:28Z

Basically what I've changed is created three new style objects:

CheckableStyle
RadioStyle
SelectableStyle

Each CliMenu keeps a copy of each of these styles to use as its base for all current-level and children items (checkable, radio and selectable items).

So an example CheckableItem would be:

<?php

use PhpSchool\CliMenu\CliMenu;
use PhpSchool\CliMenu\Builder\CliMenuBuilder;
use PhpSchool\CliMenu\Style\CheckableStyle;

require_once(__DIR__ . '/../vendor/autoload.php');

$itemCallable = function (CliMenu $menu) {
    echo $menu->getSelectedItem()->getText();
};

$menu = (new CliMenuBuilder)
    ->setTitle('Select a Language')
    ->addSubMenu('Compiled', function (CliMenuBuilder $b) use ($itemCallable) {
        $b->setTitle('Compiled Languages')
            ->addCheckableItem('Rust', $itemCallable)
            ->addCheckableItem('C++', $itemCallable)
            ->addCheckableItem('Go', $itemCallable)
            ->addCheckableItem('Java', $itemCallable)
            ->addCheckableItem('C', $itemCallable)
        ;
    })
    ->addSubMenu('Interpreted', function (CliMenuBuilder $b) use ($itemCallable) {
        $b->setTitle('Interpreted Languages')
            ->checkableStyle(function (CheckableStyle $style) {
                $style->setMarkerOff('[○] ')
                    ->setMarkerOn('[●] ');
            })
            ->addCheckableItem('PHP', $itemCallable)
            ->addCheckableItem('Javascript', $itemCallable)
            ->addCheckableItem('Ruby', $itemCallable)
            ->addCheckableItem('Python', $itemCallable)
        ;
    })
    ->build();

$menu->open();

The style classes only care about styles related to their item type. Right now they're identical, only difference are the marker texts.

You'll be able to deprecate the following in MenuStyle:

$selectedMarker
$unselectedMarker
$itemExtra
$displaysExtra

since these are item-level styles. Note that not all item types have moved off of MenuStyle - this is just the start of the PR, pending feedback.

Since the style classes all share the same interface, we're able to get rid of setUncheckedMarker, getUnchecked and similar.

Aside from this, I was able to fix the #201 issue.

CliMenuBuilder::addSubMenu() and CliMenuBuilder::addSubMenuFromBuilder() now inject a closure to MenuMenuItem(). Then, when the menu is first opened and MenuMenuItem::showSubMenu() is called then the closure is unpackaged and the menu generated.

This helps avoid creating the menu code ahead of time, and uses style values that are set after all PHP code has been read.

codecov-io · 2019-12-18T21:44:27Z

Codecov Report

Merging #203 into master will decrease coverage by 2.05%.
The diff coverage is 85.04%.

@@             Coverage Diff              @@
##             master     #203      +/-   ##
============================================
- Coverage     93.91%   91.85%   -2.06%     
- Complexity      516      554      +38     
============================================
  Files            29       35       +6     
  Lines          1561     1744     +183     
============================================
+ Hits           1466     1602     +136     
- Misses           95      142      +47

Impacted Files	Coverage Δ	Complexity Δ
src/MenuItem/RadioItem.php	`100% <100%> (ø)`	`6 <2> (-4)`	⬇️
src/MenuItem/SelectableTrait.php	`100% <100%> (ø)`	`11 <2> (+2)`	⬆️
src/Style/SelectableStyle.php	`100% <100%> (ø)`	`3 <3> (?)`
src/Style/RadioStyle.php	`100% <100%> (ø)`	`3 <3> (?)`
src/CliMenu.php	`95.34% <100%> (+0.29%)`	`115 <6> (+6)`	⬆️
src/MenuItem/CheckableItem.php	`100% <100%> (ø)`	`5 <4> (-4)`	⬇️
src/Style/CheckableStyle.php	`100% <100%> (ø)`	`3 <3> (?)`
src/MenuItem/ToggableTrait.php	`100% <100%> (ø)`	`15 <6> (+4)`	⬆️
src/MenuStyle.php	`96.61% <100%> (-0.67%)`	`60 <0> (-18)`
src/MenuItem/SplitItem.php	`100% <100%> (ø)`	`46 <1> (-1)`	⬇️
... and 15 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 a9e6a34...b08a081. Read the comment docs.

jtreminio · 2019-12-18T21:45:02Z

createMenuClosure() should be refactored into a trait or something else. IT's identical between CliMenuBuilder and SplitItemBuilder.

jtreminio · 2019-12-19T04:21:02Z

This PR provides item-level styles, and completely removes these same styles from MenuStyle.

To run a quick test:

<?php

use PhpSchool\CliMenu\CliMenu;
use PhpSchool\CliMenu\Builder\CliMenuBuilder;
use PhpSchool\CliMenu\MenuItem\SelectableItem;

require_once(__DIR__ . '/../vendor/autoload.php');

$itemCallable = function (CliMenu $menu) {
    echo $menu->getSelectedItem()->getText();
};

$item1 = new SelectableItem('Item 1', function () {});
$item1->getStyle()
    ->setMarkerOff('! ')
    ->setMarkerOn('1 ');

$item2 = new SelectableItem('Item 2', function () {});
$item2->getStyle()
    ->setMarkerOff('@ ')
    ->setMarkerOn('2 ');

$item3 = new SelectableItem('Item 3', function () {});
$item3->getStyle()
    ->setMarkerOff('# ')
    ->setMarkerOn('3 ');

$menu = (new CliMenuBuilder)
    ->setTitle('Basic CLI Menu')
    ->addMenuItem($item1)
    ->addMenuItem($item2)
    ->addMenuItem($item3)
    ->addLineBreak('-')
    ->build();

$menu->open();

AydinHassan · 2019-12-19T16:42:13Z

examples/checkable-item.php

@@ -22,8 +23,10 @@
    })
    ->addSubMenu('Interpreted', function (CliMenuBuilder $b) use ($itemCallable) {
        $b->setTitle('Interpreted Languages')
-            ->setUncheckedMarker('[○] ')
-            ->setCheckedMarker('[●] ')
+            ->setCheckableStyle(function (CheckableStyle $style) {


This is irking me a little. Can we change CheckableItem to CheckboxItem to match RadioItem please, then possibly change this to CheckboxStyle and ->setCheckboxStyle. Can we rename the checkbox as a separate PR though. This one in a bit unruly!

Yes, removing the able from items makes sense and it was bothering me as well.

As for this being unruly - I can yank out the bits with Closure that fixes nested styles not being applied, and put it into a separate PR. Anything else you think isn't clear, though?

That would be very helpful, I'll try pick out and comment on things I think can be separated. Appreciate all the work though!

AydinHassan · 2019-12-19T16:45:00Z

src/MenuItem/ItemStyleInterface.php

+
+use PhpSchool\CliMenu\Style;
+
+interface ItemStyleInterface


Can we rename this StyleableItem

StyleableItem or StyleableItemInterface?

I think we can just go with StyleableItem

Thinking about it again, we also don't need this. I think we can drop it. Each item can still implement getStyle and setStyle but should type hint on the concrete style they need. If it turns out that a few different types of item use basically the same styles then we can introduce an interface but I think until then there is no benefit.

src/Style/Colour.php

AydinHassan · 2019-12-19T16:57:44Z

src/Style/ItemStyleInterface.php

+
+namespace PhpSchool\CliMenu\Style;
+
+interface ItemStyleInterface


I think I'd rather get rid of the interfaces for each item style type. Each item renders itself so therefore can just hint on the concrete. I don't think we need interfaces for that.

AydinHassan · 2019-12-19T17:15:44Z

What I'd like to see here (after changing the checkableitem to checkbox item) a PR just changing checkbox item to this new individual style design. And then we can PR each item type one by one making it easier to review. I think it makes it easier to discuss the design that way. What do you think?

jtreminio · 2019-12-19T17:32:33Z

What I'd like to see here (after changing the checkableitem to checkbox item) a PR just changing checkbox item to this new individual style design. And then we can PR each item type one by one making it easier to review. I think it makes it easier to discuss the design that way. What do you think?

I'll redo the PR to slim it down in scope, but doing only Checkbox styling and leaving Radio and Selectable will be ugly as will need to do a bunch if if (... instanceof ...) checks.

I'll create the minimum for the Selectable and Radio styles and include in PR.

Item-level style example; fixes disappearing styles for nested

78c8150

Code cleanup

cf5816c

jtreminio added 4 commits December 18, 2019 20:09

Updates tests to use Item Style classes

cc0b968

Inject CliMenu-level style into items

f1683dc

Do not handle fg/bg at item-level just yet

036b1e9

All tests green

bbe7ad8

jtreminio mentioned this pull request Dec 19, 2019

Item-specific styling #200

Closed

Changes name for style callback methods

08e197d

AydinHassan reviewed Dec 19, 2019

View reviewed changes

src/Style/Colour.php Outdated Show resolved Hide resolved

AydinHassan reviewed Dec 19, 2019

View reviewed changes

Removes Style\Color

b08a081

jtreminio closed this Dec 19, 2019

jtreminio mentioned this pull request Dec 19, 2019

Checkbox + Radio item-level styling #204

Closed

AydinHassan mentioned this pull request Dec 19, 2019

Checkbox + Radio item-level styling #205

Closed

jtreminio deleted the feature/item-level-style branch December 20, 2019 00:59

jtreminio mentioned this pull request Dec 20, 2019

Checkbox + Radio item-level styling (3rd try) #211

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Item-level style example; fixes disappearing styles for nested #203

Item-level style example; fixes disappearing styles for nested #203

Uh oh!

jtreminio commented Dec 18, 2019

Uh oh!

jtreminio commented Dec 18, 2019 •

edited

Loading

Uh oh!

codecov-io commented Dec 18, 2019 •

edited

Loading

Uh oh!

jtreminio commented Dec 18, 2019

Uh oh!

jtreminio commented Dec 19, 2019

Uh oh!

AydinHassan Dec 19, 2019 •

edited

Loading

Uh oh!

jtreminio Dec 19, 2019

Uh oh!

AydinHassan Dec 19, 2019

Uh oh!

AydinHassan Dec 19, 2019

Uh oh!

jtreminio Dec 19, 2019

Uh oh!

AydinHassan Dec 19, 2019

Uh oh!

AydinHassan Dec 19, 2019

Uh oh!

Uh oh!

AydinHassan Dec 19, 2019

Uh oh!

AydinHassan commented Dec 19, 2019

Uh oh!

jtreminio commented Dec 19, 2019

Uh oh!

Uh oh!


		namespace PhpSchool\CliMenu\Style;

		interface ItemStyleInterface

Item-level style example; fixes disappearing styles for nested #203

Item-level style example; fixes disappearing styles for nested #203

Uh oh!

Conversation

jtreminio commented Dec 18, 2019

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

jtreminio commented Dec 18, 2019

Uh oh!

jtreminio commented Dec 19, 2019

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!

jtreminio Dec 19, 2019

Choose a reason for hiding this comment

Uh oh!

AydinHassan Dec 19, 2019

Choose a reason for hiding this comment

Uh oh!

AydinHassan Dec 19, 2019

Choose a reason for hiding this comment

Uh oh!

jtreminio Dec 19, 2019

Choose a reason for hiding this comment

Uh oh!

AydinHassan Dec 19, 2019

Choose a reason for hiding this comment

Uh oh!

AydinHassan Dec 19, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AydinHassan Dec 19, 2019

Choose a reason for hiding this comment

Uh oh!

AydinHassan commented Dec 19, 2019

Uh oh!

jtreminio commented Dec 19, 2019

Uh oh!

Uh oh!

jtreminio commented Dec 18, 2019 •

edited

Loading

codecov-io commented Dec 18, 2019 •

edited

Loading

AydinHassan Dec 19, 2019 •

edited

Loading