-
Notifications
You must be signed in to change notification settings - Fork 9
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
Add margin/border support #43
Conversation
Hey! sounds like a good idea. Would this off by default? wonder if a user would expect a filled character or one with background color to fill all the way? No need for bottom/right margin? |
so I thought we could have an option like already added the required logic, what bothers me a bit is how we need to define custom types multiple times in different places (see e.g. I considered defining them in |
I think it's perfectly fine and quite common to have a go package (a directory basically) for just one type, even if very simple and small. That is also sometimes the only way to not cause cyclic package dependencies which is not allowed in go. Not sure i have any great idea for how a "dimension" type could be done, could get some inspiration from how CSS does it? what you want is a type that stores with/height and what unit it is in? no conversions needed etc? maybe width/height as ints and a enum for unit, possibly some Having |
Ditched the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! maybe fix up the wip commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update README with new option?
instead of CSS "margin" style; is simpler and more robust
Renamed WIP commit; updated README. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! i'm ok to merge if you ready
This is a quick hack to add some "breathing room" around the image contents, to avoid that the characters touch the edge of the image. (I believe it's more readable and more pleasant to look at). It turns out that, once again, the easy way to achieve this with
border=...
does not work across all viewers 🤡 so we have to do some gymnastics offsetting the contents withmargin
and raising the image size accordingly so that no contents get clipped off.So this is a quick hack with hard coded 10px margin; it only works when using charbox mode (b/c we cannot mix
px
andem
for the image size obviously).If you like it we can add a
margin
argument or similar; not sure yet how to handle it in non-charbox mode; it may be a bit of a pain but I believe it will be worth it.