badges / poser

PHP badges library
https://poser.pugx.org
MIT License
124 stars 43 forks source link

Add ability to change label color #80

Open Fabien-jrt opened 2 years ago

Fabien-jrt commented 2 years ago

Feature Request

Q A
New Feature yes
RFC no
BC Break no

Summary

As far as I know there is no way to change the label color (left part).

I think it might be a great feature to be able to add a ?label_color=FFFFFF to further customize the badge. With FFFFFF: the value of the color in hex.

Shields.io support this (See 'styles' on shields.io): Screenshot 2022-01-15 at 15 13 21

So it would give something like this:

echo $poser->generate('license', 'MIT', 'FFFFFF', '428F7E', 'plastic');
// or
$image = $poser->generate('license', 'MIT', 'FFFFFF', '428F7E', 'plastic');

The default color should be the default grey/gray.


Original: antonkomarev/github-profile-views-counter #40.

garak commented 2 years ago

I think that every new option should be added in the end, with a default value. Otherwise, it would be a BC break

Fabien-jrt commented 2 years ago

I think that every new option should be added in the end, with a default value. Otherwise, it would be a BC break

I totally agree. The function should look more like this:

generate($label, $text, $text_color, $label_color = DEFAULT_LABEL_COLOR)

It will avoid the BC break.

However, even though I am for this implementation, I think it's a bit confusing for the user to write label, text, text_color, label_color. As a user, it seems more natural to me to write in this order: label, text, label_color, text_color.

antonkomarev commented 2 years ago

Named arguments can fix this 😉 Many people say it's an anti-pattern, but still one of the possible solutions.

antonkomarev commented 2 years ago

Other solution - encapsulate all arguments in Value Objects.

generate(
    new Label($text, $textColor, $bgColor),
    new Text($text, $textColor, $bgColor),
    new BadgeStyle('plastic')
)

Then values validation will be performed inside of the VOs, what may simplify the code.

AlessandroMinoccheri commented 2 years ago

IMHO I prefer Value Objects, to explicit arguments.

Using Value Objects increases testability and it's more clear for me.

garak commented 2 years ago

IMHO I prefer Value Objects, to explicit arguments.

Using Value Objects increases testability and it's more clear for me.

I agree. We can easily change the current method to accept both (a string or a VO), so we can ensure BC

JellyBellyDev commented 2 years ago

I am not crazy about the mixed solution!
What do you think if we plan a major release by switching to the ValueObjects approach?

garak commented 2 years ago

I am not crazy about the mixed solution! What do you think if we plan a major release by switching to the ValueObjects approach?

Yeah, it's a compromise. I don't like so much the idea of a major only to add a feature

JellyBellyDev commented 2 years ago

It is not the only feature addition, but it is a re-design of the api to better support all features. No? :)

antonkomarev commented 2 years ago

New major release could add logo to badge too #77

AlessandroMinoccheri commented 2 years ago

IMHO I think it's better to switch to the Value Objects approach. It's a refactoring that adds a new feature. We can also add other things in this way.

JellyBellyDev commented 2 years ago

who would like to take charge of the improvement?