Split item with builder #127
Conversation
Codecov Report
@@ Coverage Diff @@
## master #127 +/- ##
============================================
- Coverage 97.09% 96.87% -0.22%
- Complexity 361 428 +67
============================================
Files 23 26 +3
Lines 1135 1311 +176
============================================
+ Hits 1102 1270 +168
- Misses 33 41 +8
Continue to review full report at Codecov.
|
src/MenuItem/SplitItem.php
Outdated
| */ | ||
| public function getText() : string | ||
| { | ||
| $text = []; |
There was a problem hiding this comment.
I think we should just throw an exception in here - there is not much point to it is there? BadMethodCallException or something
There was a problem hiding this comment.
I agree but since its a function the interface has, I guess you would expect all items to return a string. No ?
There was a problem hiding this comment.
Technically yes, but fulfilling the interface with some strange combination is useless. In any case it's not possible to actually receive this item in an action callback I don't think? It's more of an implementation detail, so imo it's okay to throw an exception here.
src/MenuItem/SplitItem.php
Outdated
| /** | ||
| * @var int | ||
| */ | ||
| private $selectedItemIndex; |
There was a problem hiding this comment.
this seems to accept null also int|null
src/MenuItem/SplitItem.php
Outdated
|
|
||
| $lines = 0; | ||
| $cells = []; | ||
| foreach ($this->items as $index => $item) { |
There was a problem hiding this comment.
looks like this could be an array_map also
src/MenuItem/SplitItem.php
Outdated
| sprintf('%s%s', $marker, $item->getText()), | ||
| $length | ||
| ); | ||
| $cell = array_map(function ($row) use ($length, $style, $isSelected) { |
There was a problem hiding this comment.
Maybe extract this to a private method? There is quite a lot going on in here.
src/MenuItem/SplitItem.php
Outdated
| } | ||
| $cells[] = $cell; | ||
| } | ||
|
|
There was a problem hiding this comment.
lines could just become: $lines = max(array_map('count', $cells)); getting rid of a few temporary variables.
src/MenuItem/SplitItem.php
Outdated
|
|
||
| public function getSelectedItem() : MenuItemInterface | ||
| { | ||
| return $this->items[$this->selectedItemIndex]; |
There was a problem hiding this comment.
What happens if $this->selectedItemIndex is null? Can that happen?
src/MenuItem/SplitItem.php
Outdated
|
|
||
| /** | ||
| * Can the item be selected | ||
| * Not really in this case but that's the trick |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
src/CliMenu.php
Outdated
| { | ||
| return $this->items[$this->selectedItem]; | ||
| if ($oneLevelDeep) { |
There was a problem hiding this comment.
Could you maybe explain this method a little? I don't understand :(
There was a problem hiding this comment.
Maybe you can revert the changes here and create a method isSelectedItemASplitItem which you can call in the above method instead of
$item = $this->getSelectedItem(true);
if (!$item instanceof SplitItem) {
return;
}
Which will then look like:
if (!$this->isSelectedItemASplitItem()) {
return;
}
$item = $this->getSelectedSplitItem();There was a problem hiding this comment.
I made it so that getSelectedItem returns the right element.
It means that if we are selecting an item (let's say a SelectableItem) which is inside a SplitItem, it will return the SelectableItem and not the SplitItem.
But what if we want the "first level" selected element (so SplitItem in our example) ? Then we set the argument to true and the function will not "dig" layers to find the right one.
If we one day allow split items inside split items or something else, then it would still work as intended.
If we add some new element that allows multiple item inside itself then it would still do its job too.
The point is not to check if the element is a SplitItem (event though it's now used like that), but to find what is the top level element that is selected.
I agree that it is currently only used when trying to move the selection left or right, but since it's a public function it can also be used outside of the class so that (for example) a keyboard shortcut could have different effects depending on the element currently selected.
src/CliMenu.php
Outdated
| } elseif ($this->getSelectedItem()->canSelect()) { | ||
| return; | ||
| if ($direction === 'UP' || $direction === 'DOWN') { | ||
| $itemKeys = array_keys($this->items); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
src/CliMenu.php
Outdated
| return; | ||
| } | ||
| } else { | ||
| $item = $this->getSelectedItem(true); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
src/CliMenuBuilder.php
Outdated
| /** | ||
| * Injects a submenu directly (without going through the builder | ||
| */ | ||
| public function injectSubMenu(string $id, CliMenu $subMenu) : CliMenuBuilder |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Refactoring and such
Split item menu tests
Fix wordwrap
New wordwrap method
Split Item - Item Extra
tests for split item menu.
Refactor split item item selection and CliMenu can select checks.
Hey @Lynesth
I managed to get the builder working and decoupled the builder from the
SplitItem. Will do the review in this PR.