Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Dec 20, 2019
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d93994f
Checkbox item styling
jtreminio Dec 20, 2019
313d3e6
Radio item styling
jtreminio Dec 20, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -817,10 +817,13 @@ You may also change the marker for `\PhpSchool\CliMenu\MenuItem\CheckboxItem`:
<?php

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

$menu = (new CliMenuBuilder)
->setUncheckedMarker('[○] ')
->setCheckedMarker('[●] ')
->modifyCheckboxStyle(function (CheckboxStyle $style) {
$style->setMarkerOff('[○] ')
Copy link
Member

@AydinHassan AydinHassan Dec 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I prefer setCheckedMarker and setUncheckedMarker. We can address that later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was hoping to keep the naming scheme identical across Selectable, Checkbox and Radio items.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, since removed all the interfaces it doesn't matter much anymore.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've got PR for SelectableItem styling ready. Should I hold off on it, or should I submit? The code will follow this PR.

->setMarkerOn('[●] ');
})
->build();
```

Expand All @@ -830,10 +833,13 @@ and for `\PhpSchool\CliMenu\MenuItem\RadioItem`:
<?php

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

$menu = (new CliMenuBuilder)
->setUnradioMarker('[ ] ')
->setRadioMarker('[✔] ')
->modifyRadioStyle(function (RadioStyle $style) {
$style->setMarkerOff('[ ] ')
->setMarkerOn('[✔] ');
})
->build();
```

Expand Down
7 changes: 5 additions & 2 deletions examples/checkable-item.php → examples/checkbox-item.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

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

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

Expand All @@ -22,8 +23,10 @@
})
->addSubMenu('Interpreted', function (CliMenuBuilder $b) use ($itemCallable) {
$b->setTitle('Interpreted Languages')
->setUncheckedMarker('[○] ')
->setCheckedMarker('[●] ')
->modifyCheckboxStyle(function (CheckboxStyle $style) {
$style->setMarkerOff('[○] ')
->setMarkerOn('[●] ');
})
->addCheckboxItem('PHP', $itemCallable)
->addCheckboxItem('Javascript', $itemCallable)
->addCheckboxItem('Ruby', $itemCallable)
Expand Down
7 changes: 5 additions & 2 deletions examples/radio-item.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

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

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

Expand All @@ -22,8 +23,10 @@
})
->addSubMenu('Interpreted', function (CliMenuBuilder $b) use ($itemCallable) {
$b->setTitle('Interpreted Languages')
->setUnradioMarker('[ ] ')
->setRadioMarker('[✔] ')
->modifyRadioStyle(function (RadioStyle $style) {
$style->setMarkerOff('[ ] ')
->setMarkerOn('[✔] ');
})
->addRadioItem('PHP', $itemCallable)
->addRadioItem('Javascript', $itemCallable)
->addRadioItem('Ruby', $itemCallable)
Expand Down
0 examples/split-checkable-item.php → examples/split-checkbox-item.php
View file
Open in desktop
File renamed without changes.
92 changes: 62 additions & 30 deletions src/Builder/CliMenuBuilder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
use PhpSchool\CliMenu\MenuItem\SplitItem;
use PhpSchool\CliMenu\MenuItem\StaticItem;
use PhpSchool\CliMenu\MenuStyle;
use PhpSchool\CliMenu\Style\CheckboxStyle;
use PhpSchool\CliMenu\Style\RadioStyle;
use PhpSchool\CliMenu\Terminal\TerminalFactory;
use PhpSchool\Terminal\Terminal;

Expand Down Expand Up @@ -187,7 +189,7 @@ public function addSubMenu(string $text, \Closure $callback) : self

$menu = $builder->build();
$menu->setParent($this->menu);

$this->menu->addItem($item = new MenuMenuItem(
$text,
$menu,
Expand Down Expand Up @@ -290,7 +292,7 @@ public function addSplitItem(\Closure $callback) : self
}

$callback($builder);

$this->menu->addItem($splitItem = $builder->build());

$this->processSplitItemShortcuts($splitItem);
Expand Down Expand Up @@ -405,34 +407,6 @@ public function setSelectedMarker(string $marker) : self
return $this;
}

public function setUncheckedMarker(string $marker) : self
{
$this->style->setUncheckedMarker($marker);

return $this;
}

public function setCheckedMarker(string $marker) : self
{
$this->style->setCheckedMarker($marker);

return $this;
}

public function setUnradioMarker(string $marker) : self
{
$this->style->setUnradioMarker($marker);

return $this;
}

public function setRadioMarker(string $marker) : self
{
$this->style->setRadioMarker($marker);

return $this;
}

public function setItemExtra(string $extra) : self
{
$this->style->setItemExtra($extra);
Expand Down Expand Up @@ -551,6 +525,44 @@ public function build() : CliMenu
return $this->menu;
}

public function getCheckboxStyle() : CheckboxStyle
{
return $this->menu->getCheckboxStyle();
}

public function setCheckboxStyle(CheckboxStyle $style) : self
{
$this->menu->setCheckboxStyle($style);

return $this;
}

public function modifyCheckboxStyle(callable $itemCallable) : self
{
$itemCallable($this->menu->getCheckboxStyle());

return $this;
}

public function getRadioStyle() : RadioStyle
{
return $this->menu->getRadioStyle();
}

public function setRadioStyle(RadioStyle $style) : self
{
$this->menu->setRadioStyle($style);

return $this;
}

public function modifyRadioStyle(callable $itemCallable) : self
{
$itemCallable($this->menu->getRadioStyle());

return $this;
}

/**
* Pass styles from current menu to sub-menu
* only if sub-menu style has not be customized
Expand All @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

&& !$item->getStyle()->hasChangedFromDefaults()
) {
$item->setStyle(clone $menu->getCheckboxStyle());
}

if ($item instanceof RadioItem
&& !$item->getStyle()->hasChangedFromDefaults()
) {
$item->setStyle(clone $menu->getRadioStyle());
}
Comment on lines +575 to +585
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this and suspect it will get uglier. But also I'm happy for us to fix this later.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean refactor, just because of all the instanceof checks.


// Apply current style to children, if they are not customized
if ($item instanceof MenuMenuItem) {
$subMenu = $item->getSubMenu();
Expand All @@ -568,6 +592,14 @@ private function propagateStyles(CliMenu $menu, array $items = [])
$subMenu->setStyle(clone $menu->getStyle());
}

if (!$subMenu->getCheckboxStyle()->hasChangedFromDefaults()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each submenu object gets a copy of radio and checkbox style object to keep. This is what is handed out to children items.

$subMenu->setCheckboxStyle(clone $menu->getCheckboxStyle());
}

if (!$subMenu->getRadioStyle()->hasChangedFromDefaults()) {
$subMenu->setRadioStyle(clone $menu->getRadioStyle());
}

$this->propagateStyles($subMenu);
}

Expand Down
6 changes: 3 additions & 3 deletions src/Builder/SplitItemBuilder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,14 +113,14 @@ public function addSubMenu(string $text, \Closure $callback) : self
$menu,
$builder->isMenuDisabled()
));

return $this;
}

public function setGutter(int $gutter) : self
{
$this->splitItem->setGutter($gutter);

return $this;
}

Expand All @@ -134,7 +134,7 @@ public function enableAutoShortcuts(string $regex = null) : self

return $this;
}

public function build() : SplitItem
{
return $this->splitItem;
Expand Down
46 changes: 42 additions & 4 deletions src/CliMenu.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
use PhpSchool\CliMenu\MenuItem\StaticItem;
use PhpSchool\CliMenu\Dialogue\Confirm;
use PhpSchool\CliMenu\Dialogue\Flash;
use PhpSchool\CliMenu\Style\CheckboxStyle;
use PhpSchool\CliMenu\Style\RadioStyle;
use PhpSchool\CliMenu\Terminal\TerminalFactory;
use PhpSchool\CliMenu\Util\StringUtil as s;
use PhpSchool\Terminal\InputCharacter;
Expand All @@ -35,6 +37,16 @@ class CliMenu
*/
protected $style;

/**
* @var CheckboxStyle
*/
private $checkboxStyle;

/**
* @var RadioStyle
*/
private $radioStyle;

/**
* @var ?string
*/
Expand Down Expand Up @@ -90,10 +102,12 @@ public function __construct(
Terminal $terminal = null,
MenuStyle $style = null
) {
$this->title = $title;
$this->items = $items;
$this->terminal = $terminal ?: TerminalFactory::fromSystem();
$this->style = $style ?: new MenuStyle($this->terminal);
$this->title = $title;
$this->items = $items;
$this->terminal = $terminal ?: TerminalFactory::fromSystem();
$this->style = $style ?: new MenuStyle($this->terminal);
$this->checkboxStyle = new CheckboxStyle();
$this->radioStyle = new RadioStyle();

$this->selectFirstItem();
}
Expand Down Expand Up @@ -640,6 +654,30 @@ public function setStyle(MenuStyle $style) : void
$this->style = $style;
}

public function getCheckboxStyle() : CheckboxStyle
{
return $this->checkboxStyle;
}

public function setCheckboxStyle(CheckboxStyle $style) : self
{
$this->checkboxStyle = $style;

return $this;
}

public function getRadioStyle() : RadioStyle
{
return $this->radioStyle;
}

public function setRadioStyle(RadioStyle $style) : self
{
$this->radioStyle = $style;

return $this;
}

public function getCurrentFrame() : Frame
{
return $this->currentFrame;
Expand Down
Loading