php-school / cli-menu

🖥 Build beautiful PHP CLI menus. Simple yet Powerful. Expressive DSL.
http://www.phpschool.io
MIT License
1.94k stars 106 forks source link

Updated Confirmation Dialog #248

Closed ccsliinc closed 2 years ago

ccsliinc commented 2 years ago

Please look over the proposed changes to make the dialog box cancellable. The proposed changes will not break any existing code. The method remains the same with an additional argument, additional example added to source as well.

AydinHassan commented 2 years ago

Hi @ccsliinc thanks for the contribution - I've been afk for a few days, will get to this soon

codecov-commenter commented 2 years ago

Codecov Report

Merging #248 (42a6491) into master (d5a96a5) will increase coverage by 0.30%. The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #248      +/-   ##
============================================
+ Coverage     93.08%   93.38%   +0.30%     
- Complexity      634      659      +25     
============================================
  Files            37       38       +1     
  Lines          1922     1994      +72     
============================================
+ Hits           1789     1862      +73     
+ Misses          133      132       -1     
Impacted Files Coverage Δ
src/CliMenu.php 94.40% <100.00%> (+0.10%) :arrow_up:
src/Dialogue/CancellableConfirm.php 100.00% <100.00%> (ø)
src/Dialogue/Dialogue.php 90.00% <100.00%> (ø)
src/Input/InputIO.php 96.92% <100.00%> (+0.02%) :arrow_up:
src/MenuStyle.php 98.96% <100.00%> (+0.01%) :arrow_up:
src/Style/Locator.php 100.00% <0.00%> (+2.85%) :arrow_up:

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 d5a96a5...42a6491. Read the comment docs.

AydinHassan commented 2 years ago

Hi @ccsliinc - I've taken a look and while it mostly looks good I'm concerned about using the existing Confirm class. I'd prefer, as noted in the previous PR, that we use a separate class. CancellableConfirm would be fine.

ccsliinc commented 2 years ago

ok i will fix and resubmit, give me a couple days.

AydinHassan commented 2 years ago

Sure, no rush!

ccsliinc commented 2 years ago

I uploaded a fixed PR.

AydinHassan commented 2 years ago

Thanks for the contribution @ccsliinc !