Skip to content

Allow the use of callables to determine whether a menu item is disabled or not #237

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.

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

Closed
wants to merge 7 commits into from

Conversation

jackwh
Copy link
Contributor

@jackwh jackwh commented Apr 18, 2020

It's easy enough to set a menu item as disabled, but what if you want to re-enable it following a user action within the menu? I couldn't see a clean way to do this (hopefully I'm not missing something...!), so this PR should fix it.

It simply changes the method definitions for addItem, addCheckboxItem, and addRadioItem to allow the $disabled parameter to be a boolean or a callable. When an item is selected that toggles the state of another item, a call to $menu->redraw() refreshes the screen and updates the status of the item.

Here's an example of how it works. I put another simple example in the README file too.

public bool $isChecked = false;

public function showMenu(): void
{
    $itemSelected     = function (CliMenu $menu) {
        $menu->redraw();
        $menu->flash('Item selected!')->display();
    };
    $checkboxSelected = function (CliMenu $menu) use ($itemSelected) {
        $this->isChecked = true;
        $itemSelected($menu);
    };

    (new CliMenuBuilder)
        ->setTitle('Callable Functions')
        ->addCheckboxItem(
            'Check this box...',
            $checkboxSelected,
            false,
            fn() => $this->isChecked
        )->addItem(
            'This item is enabled if the checkbox is not ticked',
            $itemSelected,
            false,
            fn() => $this->isChecked
        )->addRadioItem(
            'These radio buttons are enabled if the checkbox is selected',
            $itemSelected,
            false,
            fn() => ! $this->isChecked
        )->addRadioItem(
            'These radio buttons are enabled if the checkbox is selected',
            $itemSelected,
            false,
            fn() => ! $this->isChecked
        )
        ->build()
        ->open();
}

@AydinHassan
Copy link
Member

I'm not entirely sold on this proposal as I think it makes the builder even more complicated and I already think there are too many parameters there, mixing the types of one one is even more confusing.

I think my approach to this problem would be to make the items' enabled/disabled status toggle-able. Then if you wanted to have some items depend on others you could you just keep a reference to those items inside your item and toggle their status there:

For example:

$itemSelected = function (CliMenu $menu) {
    $menu->redraw();
    $menu->flash('Item selected!')->display();
};

$item = new SelectableItem('This item is enabled if the checkbox is not ticked', $itemSelected);
$radio1 = new RadioItem('These radio buttons are enabled if the checkbox is selected', $itemSelected, false, true);
$radio2 = new RadioItem('These radio buttons are enabled if the checkbox is selected', $itemSelected, false, true);

$checkboxSelected = function (CliMenu $menu) use ($itemSelected, $item, $radio1, $radio2) {
    //could even just have a `toggleDisabled()` on all items with no need to check the current items status
    //needs a better name though
    if ($menu->getSelectedItem()->getChecked()) {
        $item->disable();
        $radio1->enable();
        $radio2->enable();
    }

    $itemSelected($menu);
};

$checkBoxItem = new CheckboxItem('Check this box...', $checkboxSelected);

(new CliMenuBuilder)
    ->setTitle('Callable Functions')
    ->addMenuItem($checkBoxItem)
    ->addMenuItem($item)
    ->addMenuItem($radio1)
    ->addMenuItem($radio2)
    ->build()
    ->open();

It's not quite as slick as your proposal, but I think it keeps the builder simpler, albeit offloading some complexity to the consumer, which I am fine with, since this is probably an edge case.

What do you think @jackwh ?

\cc @mikeymike for your thoughts

@AydinHassan
Copy link
Member

Feel free to rework if you find the time @jackwh. Closing for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants