Skip to content
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

Detect Ascii art that does not fit in the menu #63

Merged
merged 2 commits into from
Mar 1, 2017
Merged

Detect Ascii art that does not fit in the menu #63

merged 2 commits into from
Mar 1, 2017

Conversation

robertmarsal
Copy link
Contributor

Hello,

This PR provides a solution for the request made in #5.

Thanks,
Rob

@codecov-io
Copy link

codecov-io commented Mar 1, 2017

Codecov Report

Merging #63 into master will increase coverage by 0.06%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master      #63      +/-   ##
============================================
+ Coverage      88.7%   88.76%   +0.06%     
- Complexity      237      239       +2     
============================================
  Files            18       18              
  Lines           726      730       +4     
============================================
+ Hits            644      648       +4     
  Misses           82       82
Impacted Files Coverage Δ Complexity Δ
src/CliMenuBuilder.php 98.69% <100%> (+0.01%) 44 <0> (+1)
src/MenuItem/AsciiArtItem.php 100% <100%> (ø) 12 <1> (+1)

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 9e7c884...1eee3d6. Read the comment docs.

@mikeymike
Copy link
Member

hey @robertboloc thanks for the contribution 😄 ... looking back at this I personally think it might be best that we don't throw an exception as this might be unwanted behaviour, e.g. if I build an app but I don't expect someone to be using it with a terminal width less than 20 but someone does and they get an exception that doesn't quite make sense to them as a user. What do you think ?

I'm personally leaning more towards silently not adding the ascii art or replacing it with a small ascii art sign (something to represent it being an image but a failed one just for amusement really) ... but both would represent keeping functionality for the end user, just sacrificing aesthetics in most cases.

@AydinHassan have you got any thoughts on this ?

@robertmarsal
Copy link
Contributor Author

hi @mikeymike that makes sense, I added the exception thinking of it from a development point of view, but you are right that for the end user it's a bad experience. I will skip adding the ascii art if it fails the check. That will still allow using the app without the art.

Copy link
Member

@mikeymike mikeymike left a comment

Choose a reason for hiding this comment

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

Looks good to me @robertboloc Thanks!

@mikeymike mikeymike merged commit f08d25e into php-school:master Mar 1, 2017
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