Added SvgForTheBadgeRenderer #86
Conversation
JellyBellyDev
requested review from
AlessandroMinoccheri,
JellyBellyDev and
garak
|
Hi @ricardoboss! Thanks for your PR! Can you fix the CI, please? |
|
@JellyBellyDev fixed (I hope) |
| }, | ||
| { | ||
| "type": "vcs", | ||
| "url": "https://github.com/ricardoboss/easysvg" |
There was a problem hiding this comment.
do we really need to depend on the unpublished fork of the third party lib?
There was a problem hiding this comment.
What do you mean by unpublished?
I specifically explained why this fork has a release: https://github.com/ricardoboss/easysvg/releases/tag/2.0
We need this fork, since the original library has a lot of deprecation warnings (and even fatal errors) because it supported very old PHP versions. In my fork, I fixed them and opened a PR. I am in talks with the creator of the library to merge my PR and then we can drop this extra repository.
There was a problem hiding this comment.
Nice! I pinged kartsims/easysvg#32 (comment) :)
|
@ricardoboss thanks for pushing this forward! Why Roboto and Montserrat fonts are used?
Output image looks blurred and not flat (it has some kind of bevel border on the top). |
| } | ||
| ], | ||
| "require": { | ||
| "php": ">=7.4", | ||
| "ext-gd": "*", | ||
| "ext-simplexml": "*", | ||
| "kartsims/easysvg": "^2.0", |
There was a problem hiding this comment.
Do we really need extra dependency? How svg rendering is implemented in other badge styles without it?
There was a problem hiding this comment.
In https://github.com/ekfuhrmann/badge-generator it is implemented using opentype.js. We need some way to render a font in SVG, since not everyone has the required fonts installed on their systems. If you know another way, let me know!
There was a problem hiding this comment.
No problem for me to new deps but i prefer to use the standard font as talk by @antonkomarev here: #86 (comment)
I used the same fonts that are used by the original "for the badge" generator: https://github.com/ekfuhrmann/badge-generator
The bevel on top is just my poor screenshot of the badge :) The svg itself is flat. |
| } | ||
| ], | ||
| "require": { | ||
| "php": ">=7.4", | ||
| "ext-gd": "*", | ||
| "ext-simplexml": "*", | ||
| "kartsims/easysvg": "^2.0", |
There was a problem hiding this comment.
No problem for me to new deps but i prefer to use the standard font as talk by @antonkomarev here: #86 (comment)
| }, | ||
| { | ||
| "type": "vcs", | ||
| "url": "https://github.com/ricardoboss/easysvg" |
There was a problem hiding this comment.
Nice! I pinged kartsims/easysvg#32 (comment) :)
|
@antonkomarev I addressed your comments in the last commits. I took a look at the shields.io
|
There was a problem hiding this comment.
Thanks @ricardoboss!
For now i merged but i open a issue to remove the unpublished fork deps as soon as the PR is released
Hi! In another project using this library, I requested to add support for more styles: PUGX/badge-poser#705. (PR to fix that one: PUGX/badge-poser#707)
All styles were already there, except from
for-the-badge. So I implemented it in this PR. This is what the new style looks like:Note: there is a new dependency on kartsims/easysvg. The only problem this library has, is that it still supports PHP 5.6 (oof). Maybe no one bothered to update it or the author wants to support older PHP version. Anyhow, I created a fork of it, which I added as a VCS repository in
composer.json. That way, this PR doesn't have to wait until my PR over there is merged: kartsims/easysvg#32Let me know what you think!