-
-
Notifications
You must be signed in to change notification settings - Fork 13
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
More customizable themes #13
Conversation
It's easier this way
It feels unnecessary. Ultimately a background is as simple as a path to a file or URL... so there's no real need to create a whole new class just for a new background
The underlying encode() method will only accept an instance of an EncoderInterface, so passing a string here just doesn't make sense If the user wants a different format to PNG, they should pass an instance of the relevant encoder
# Conflicts: # tests/test.php # tests/test4.png # tests/test5.png
I'll take a closer look later today! But looks solid on first glance 👀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm personally not a fan of abstract classes, but that's an implementation detail. This would definitely be a nice addition to The OG! LGTM 👌
I know what you mean. They could just be more concrete base classes. I may refactor those out at some point, now that we have pretty solid interfaces across the board. |
A variation of #7 that goes a little further.
I thought about it a lot and realised that even though I want to provide an opinionated base set of styles, it does make sense to make it all more configurable.
I really liked @svenluijten's approach of moving towards more objects and interfaces as this is clean and predictable.
Fonts and Backgrounds were the main targets. With this new approach, it's now possible for developers to use completely custom font and background classes simply by implementing the
SimonHamp\TheOg\Interfaces\Font
andSimonHamp\TheOg\Interfaces\Background
interfaces respectively.Specifically though, the differences to #7 are:
Removing the
Font
enum altogether.Relying on a single font class per font ultimately feels cleaner and more flexible. Putting all the fonts we might add to the core of this package in a single enum could get messy.
Using static methods as factories provides a similar API to the enum.
Keeping the
Background
enum, but having it construct instances of a newBackground
class, rather than the enum itself implementing the interface.This allows for the best of all scenarios:
SimonHamp\TheOg\Theme\Background
class:Interfaces\Background
interface or extend theTheme\Background
class and build some custom logic for handling their backgroundWith background images via URL already a part of the package, which also introduces background images as a cover image (in addition to the pre-existing intended to be used as repeating patterns), there may also be some opportunity in the future to consolidate how these different types of background are handled all into this single
Background
class.In addition, this PR:
Ditches the
Layout
enumThis wasn't really adding any value and felt like it would only add a burden to maintainers to remember to update when we add more layouts.
To set a layout, I think it's clear enough that a new instance of the desired layout should be passed to the image, e.g.:
Type-hints the
EncoderInterface
onImage::save()
This tightens the restriction on what kind of 'format' can be used when saving the rendered image to a file: if the user wants something other than a PNG, they must pass an instance of one of the
Intervention\Image\Encoders