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.

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

Jump to bottom
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 •
edited
Loading

edit: Ready for merging.

Related to #185

@codecov-io
Copy link

codecov-io commented Dec 16, 2019 •
edited
Loading

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.

@jtreminio jtreminio mentioned this pull request Dec 16, 2019
Closed
AydinHassan
AydinHassan reviewed Dec 16, 2019
View reviewed changes
AydinHassan
AydinHassan reviewed Dec 16, 2019
View reviewed changes
AydinHassan
AydinHassan reviewed Dec 16, 2019
View reviewed changes
AydinHassan
AydinHassan reviewed Dec 16, 2019
View reviewed changes
AydinHassan
AydinHassan reviewed Dec 16, 2019
View reviewed changes
AydinHassan
AydinHassan reviewed Dec 16, 2019
View reviewed changes
AydinHassan
AydinHassan reviewed Dec 16, 2019
View reviewed changes
AydinHassan
AydinHassan reviewed Dec 16, 2019
View reviewed changes
AydinHassan
AydinHassan reviewed Dec 16, 2019
View reviewed changes
AydinHassan
AydinHassan reviewed Dec 16, 2019
View reviewed changes
AydinHassan
AydinHassan reviewed Dec 16, 2019
View reviewed changes
AydinHassan
AydinHassan reviewed Dec 16, 2019
View reviewed changes
AydinHassan
AydinHassan reviewed Dec 16, 2019
View reviewed changes
AydinHassan
AydinHassan reviewed Dec 16, 2019
View reviewed changes
AydinHassan
AydinHassan reviewed Dec 16, 2019
View reviewed changes
AydinHassan
AydinHassan reviewed Dec 16, 2019
View reviewed changes
AydinHassan
AydinHassan reviewed Dec 16, 2019
View reviewed changes
AydinHassan
AydinHassan requested changes Dec 16, 2019
View reviewed changes
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

jtreminio commented Dec 16, 2019

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

jtreminio commented Dec 16, 2019

Pending unit tests.

AydinHassan
AydinHassan requested changes Dec 16, 2019
View reviewed changes
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
@jtreminio jtreminio mentioned this pull request Dec 16, 2019
Closed
@AydinHassan
Copy link
Member

AydinHassan commented Dec 16, 2019

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

Reviewers

@AydinHassan AydinHassan Awaiting requested review from AydinHassan

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jtreminio @codecov-io @AydinHassan