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

feat: add support for background image URL #1

Merged
merged 6 commits into from
Jan 6, 2024

Conversation

rico-vz
Copy link
Contributor

@rico-vz rico-vz commented Jan 3, 2024

Adds the ability to set a background image from a URL.

Includes some small sanity checks to make sure it's an actual link that points to an image.

Added a new test in test_with_url.php which also shows how it can be used. Image used in the test is in public domain.

If you'd like to see any changes to my implementation, let me know and I'll work on it.

Adds the ability to set a background image URL.

Added a new test in `test_with_url.php` which also shows how it can be used. Image used in the test is in public domain 🫡
src/Image.php Outdated
Comment on lines 79 to 86
if (!filter_var($backgroundURL, FILTER_VALIDATE_URL)) {
throw new \InvalidArgumentException('URL is not valid');
}

$imageInfo = @getimagesize($backgroundURL);
if (!$imageInfo) {
throw new \InvalidArgumentException('URL doesn\'t point to an image');
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some small sanity checks to make sure it's a valid image link.

Comment on lines 199 to 202
$panel->resize($this->width, $this->height, function ($constraint) {
$constraint->aspectRatio();
$constraint->upsize();
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resize the image so it looks properly in place

@rico-vz rico-vz marked this pull request as ready for review January 3, 2024 02:20
src/Image.php Outdated Show resolved Hide resolved
Copy link
Owner

@simonhamp simonhamp left a comment

Choose a reason for hiding this comment

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

This looks great! Thanks so much.

Apologies are in order from me though. Before I had a chance to review and merge this, I had embarked on a sizable refactor, so there are a couple of merge conflicts now

It shouldn't be too drastic to fix - basically I moved rendering to happen in the Layout rather than in the Image and renamed the trait from RendersImages to RendersFeatures.

This means that properties from the Image are now accessible via the $config property on the Layout, i.e.:

- $this->backgroundUrl
+ $this->config->backgroundUrl

Let me know if you have any trouble and I'll take a look. Excited to get this merged as it will be a really nice addition so early on!

src/Image.php Outdated Show resolved Hide resolved
src/Traits/RendersImages.php Outdated Show resolved Hide resolved
src/Traits/RendersImages.php Outdated Show resolved Hide resolved
src/Traits/RendersImages.php Outdated Show resolved Hide resolved
simonhamp and others added 4 commits January 5, 2024 14:25
Co-authored-by: Sven Luijten <11269635+svenluijten@users.noreply.github.com>
This commit removes the following test files:
- `tests/test.png`
- `tests/test_background_url.png`
- `tests/test_with_url.php`

These files are no longer needed and have been deleted.
@rico-vz
Copy link
Contributor Author

rico-vz commented Jan 5, 2024

This looks great! Thanks so much.

Apologies are in order from me though. Before I had a chance to review and merge this, I had embarked on a sizable refactor, so there are a couple of merge conflicts now

It shouldn't be too drastic to fix - basically I moved rendering to happen in the Layout rather than in the Image and renamed the trait from RendersImages to RendersFeatures.

This means that properties from the Image are now accessible via the $config property on the Layout, i.e.:

- $this->backgroundUrl
+ $this->config->backgroundUrl

Let me know if you have any trouble and I'll take a look. Excited to get this merged as it will be a really nice addition so early on!

It should all be working again.
If anything can be improved, please let me know!

Copy link
Owner

@simonhamp simonhamp left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for working on this 🙏🏼 looking forward to your next PR 😃

@simonhamp simonhamp merged commit 2221ff4 into simonhamp:main Jan 6, 2024
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.

None yet

3 participants