Skip to content

Initial checkable item support #186

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

Merged
merged 6 commits into from
Dec 16, 2019
Merged

Initial checkable item support #186

merged 6 commits into from
Dec 16, 2019

Conversation

jtreminio
Copy link
Contributor

@jtreminio jtreminio commented Dec 16, 2019

edit: Ready for merging.

Related to #185

@codecov-io
Copy link

codecov-io commented Dec 16, 2019

Codecov Report

Merging #186 into master will decrease coverage by 0.09%.
The diff coverage is 91.17%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master     #186     +/-   ##
===========================================
- Coverage     93.29%   93.19%   -0.1%     
- Complexity      460      486     +26     
===========================================
  Files            26       27      +1     
  Lines          1402     1470     +68     
===========================================
+ Hits           1308     1370     +62     
- Misses           94      100      +6
Impacted Files Coverage Δ Complexity Δ
src/Builder/CliMenuBuilder.php 83.67% <100%> (+0.78%) 65 <3> (+3) ⬆️
src/MenuStyle.php 97.14% <100%> (+0.15%) 74 <4> (+4) ⬆️
src/MenuItem/CheckableItem.php 86.66% <86.66%> (ø) 19 <19> (?)

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 d80c749...86416ab. Read the comment docs.

Copy link
Member

@AydinHassan AydinHassan left a comment

Choose a reason for hiding this comment

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

I think we should drop item extra support from this item, I can't imagine it being very useful in this circumstance. What do you think?

Overall it looks great. I've left a few other comments to address, mostly regarding renaming a few things. But I'd say go ahead and add tests :)

/cc @mikeymike

@jtreminio
Copy link
Contributor Author

I think we should drop item extra support from this item, I can't imagine it being very useful in this circumstance. What do you think?

Overall it looks great. I've left a few other comments to address, mostly regarding renaming a few things. But I'd say go ahead and add tests :)

/cc @mikeymike

Would prefer this be a separate PR.

@jtreminio
Copy link
Contributor Author

Pending unit tests.

Copy link
Member

@AydinHassan AydinHassan left a comment

Choose a reason for hiding this comment

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

Everything looks great now apart from one comment.

@jtreminio jtreminio changed the title Initial toggleable item support Initial checkable item support Dec 16, 2019
@AydinHassan
Copy link
Member

LGTM! Thanks a lot @jtreminio

@AydinHassan AydinHassan merged commit 3c4db90 into php-school:master Dec 16, 2019
@jtreminio jtreminio deleted the feature/toggleable-item branch December 17, 2019 18:47
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.

3 participants