Use interfaces to allow for custom backgrounds and fonts

[svenluijten]

This PR replaces several user-exposed instances of the enums with interfaces. Enums are (by definition) not extendible. This makes it hard (impossible?) for users of the package to "bring their own content".

For example: I want to set a different background than one of the ones defined in the Background enum. I can't use my own because the ->background() method on the Theme interface expects an instance of SimonHamp\TheOg\Background.

Instead of using enums, we'll use an interface that people can now implement to provide their own instances.

• Replace background enum with interface.
• Replace font enum with interface.
• Update README.

[svenluijten]

Alternatively we could create a class per font family (in this case Inter) and then use it like so:

class Inter implements Font 
{
    public function __construct(private string $variant = 'Regular') {}

    public function path(): string
    {
        return __DIR__ . '/../../resources/fonts/Inter/static/Inter-' . $this->variant . '.ttf';
    }
}

$font = new Inter('Bold');

But I'll leave that up to you. I personally quite like the customizability that having a class per font affords, but it's an option! 😄

[simonhamp]

I implemented it this way originally and then decided against it because it introduced too many possibilities for folks to create images that didn't really work or look right.

For backgrounds, it's now possible to pass a backgroundUrl to the image and set it on the theme, but I think exposing more than that (for fonts too) opens up too much optionality

The purpose of the package is to be 'opinionated' and provide a limited set of options. So I'm not going to merge this right now

Happy to keep this around to gauge interest, though

[svenluijten]

because it introduced too many possibilities for folks to create images that didn't really work or look right.

I disagree, but in the end it's up to you! I don't believe it's the repository's (or your) fault if the user misuses/abuses the options the code provides. As the code is in this PR it still defaults to the previous 3 backgrounds and 1 font family, it just opens it up for other people who know what they are doing to add their own in their own codebase.

Having an opinionated package is fine though, and I completely understand the reasoning behind it.

[simonhamp]

Yeh I understand. Let's see what the people want

[svenluijten]

Okay, I've updated it. Turns out enums can implement interfaces (weird syntax, but it works). So we can have the best of both worlds! It's still just as customizable as before, but we don't have to change up the way it's done internally.

@simonhamp what do you think of this approach? 😄

[simonhamp]

That's interesting! I wasn't even aware that enums could implement interfaces!

That said, I'm not sure if they should in this case because then the underlying behaviour is tied to two different data structures: on the one hand it could be an enum, on the other it could be a class.

That might not cause us any real problems, but it means in places where we simply set a property to the instance being passed in, it could lead to confusion about what kind of object we're dealing - I think it ends up obscuring that.

After more thought, I really liked the approach of having class-based elements because it feels like classes really do give the developer a lot of flexibility, if that's what they want. It also brings consistency to the code base, so I've really leaned into that.

I am about to pop another PR on here for you to have a look at, which takes your earlier version of this and adapts it further, to maintain the flexibility whilst retaining some of the apparent simplicity of using enums and without risking making the enum the object that gets passed around internally.

[svenluijten]

I wasn't even aware that enums could implement interfaces!

Same!

the underlying behaviour is tied to two different data structures: on the one hand it could be an enum, on the other it could be a class.

Eh, I'm still expecting the Background and Font interfaces right now. Whether that's an enum or a class would be up to the user. As long as it implements the interface it's workable 😜

I am about to pop another PR on here for you to have a look at, which takes your earlier version of this and adapts it further, to maintain the flexibility whilst retaining some of the apparent simplicity of using enums and without risking making the enum the object that gets passed around internally.

Sounds good! Shall we close this one then and continue the discussion in your PR? 😄

[simonhamp]

Whether that's an enum or a class would be up to the user. As long as it implements the interface it's workable 😜

I agree that as long as it implements the interface it's workable, but I'm not sure I'm completely comfortable with the idea that in some cases it can be an enum instance and in other cases it represents a class instance.

I just feel like there might be some dragons down that road.

🐲

Happy to be completely wrong, of course! But I'd rather avoid it for now.

Sounds good! Shall we close this one then and continue the discussion in your PR? 😄

Well, have a look and let me know what you think: #13

[simonhamp]

Closing in favour of #13