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

Solved Background color text issue #304

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

scharf-roie
Copy link

Updated Simpleimage.php but more extensive testing than using the example images can be done

Updated Simpleimage.php but more extensive testing than using the example images can be done
* Final result shows a text with the backgroundColor requested
*/

public function textWithBackgroundColor($text, $options, $backgroundColor, &$boundary = null) {
Copy link
Owner

@claviska claviska Oct 3, 2022

Choose a reason for hiding this comment

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

I'm not a fan of the naming here, as it's likely too narrow in scope. For example, I can easily see the next request being for something like textWithBackgroundImage() and before you know it, we have a bunch of custom text functions that add to the API footprint and make the library harder to work with.

Ideally, this functionality would be baked into text() using the existing $options argument:

public function text($text, $options, &$boundary = null) {
   // ...
}

This argument is already an array, so an extra property called backgroundColor seems to make sense here:

 $options = array_merge([
   'fontFile' => null,
   'size' => 12,
   'color' => 'black',
+  'backgroundColor' => null,
   'anchor' => 'center',
   'xOffset' => 0,
   'yOffset' => 0,
   'shadow' => null,
   'calculateOffsetFromEdge' => false,
   'baselineAlign' => true
 ], $options);

And then the logic you've added here can be tucked inside a conditional that runs only when $options->backgroundColor.

It would also be helpful if you wanted to add the argument to this section of the README, but I can do that if you don't want to.

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.

2 participants