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

Add ability to change label color #80

Open
Fabien-jrt opened this issue Jan 15, 2022 · 12 comments
Open

Add ability to change label color #80

Fabien-jrt opened this issue Jan 15, 2022 · 12 comments

Comments

@Fabien-jrt
Copy link

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
Copy link
Collaborator

garak commented Jan 15, 2022

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
Copy link
Author

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
Copy link
Collaborator

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

@antonkomarev
Copy link
Collaborator

antonkomarev commented Jan 15, 2022

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
Copy link
Collaborator

IMHO I prefer Value Objects, to explicit arguments.

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

@garak
Copy link
Collaborator

garak commented Jan 16, 2022

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
Copy link
Collaborator

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
Copy link
Collaborator

garak commented Jan 17, 2022

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
Copy link
Collaborator

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

@antonkomarev
Copy link
Collaborator

antonkomarev commented Jan 17, 2022

New major release could add logo to badge too #77

@AlessandroMinoccheri
Copy link
Collaborator

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
Copy link
Collaborator

who would like to take charge of the improvement?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants