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

Extensible dimension calculators #193

Merged
merged 45 commits into from
Feb 4, 2023
Merged

Extensible dimension calculators #193

merged 45 commits into from
Feb 4, 2023

Conversation

ncla
Copy link
Collaborator

@ncla ncla commented Jan 12, 2023

Described in #167, this allows developers to bind their own customized DimensionCalculator if they have different needs.

This required to move a few things around, as some things were tied to "ratios". Breakpoints have been separted of their srcSets, and I have introduced separate Source to handle it instead (it represents tag in a way). For GraphQL the ratio property will be present if using default, filesize aspect-ratio based dimension calculator.

I introduced an object class Dimensions to better contain width and height, as having an array was a bit ugly for my liking.

If developers want to customize dimension calculations, they need to implement DimensionsCalculator interface with following three methods:

  • calculateForBreakpoint for calculating multiple dimensions for breakpoint srcsets
  • calculateForImgTag is specifically targeted for width and height in <img> tag
  • calculateForPlaceholder for blurry placeholder

All of these receive Breakpoint/Source objects, so developers should have plentiful freedom when it comes to calculating dimensions, as from there you can access breakpoint params and original asset.

Here is how developers can bind their implementation in AppServiceProvider, example:

        $this->app->bind(DimensionCalculator::class, function () {
            return new class extends ResponsiveDimensionCalculator {
                public function calculateForBreakpoint(Source $source): Collection
                {
                    // Disable JPG sources
                    if ($source->getFormat() === 'original') {
                        return collect([]);
                    }

                    // Create widths from 100 to 2000 in 100px increments with fixedHeight passed from params
                    return collect(range(100, 2000, 100))
                        ->map(function ($width) use ($source) {
                            return new Dimensions($width, $source->breakpoint->params['fixedHeight']);
                        });
                }
            };
        });

The original dimension calculator has remained unchanged. I do not currently have an idea how to solve #167 width calculations elegantly, so this is up to the individual developer, now they have the freedom to do so. Or we can improve this in a later update.

Additional side-quests:

  • Fix max widths config value or glide width param not being respected in some cases
  • Fix floating numbers for width and height values for Glide endpoint, they are now rounded integers
  • Add additional tests for dimension calculator and GraphQL
  • Placeholder can be toggled per breakpoint
  • <img> tag src will now go through Glide instead of pointing to original asset file, due to DimensionCalculators ability to return specific widths and heights for <img> tag
  • Empty media attributes won't be outputted at all

Breaking changes:

  • If developers have customized .blade.php template, they will need to readjust/republish. responsive-images.blade.php now loops breakpoints and then source instead of only breakpoints which then point to srcSet, srcSetWebp and srcSetAvif.
  • GraphQL queries have changed, any project using them will have to readjust queries and handling of the response. When getting breakpoints from responsive fieldtype, breakpoints will now have sources.
  • img src attribute will now always go through Glide controller now. If users relied on filename being readable, then this will be breaking change as filename is encoded by default from Statamic Glide controller.

TODO:

  • Actually try creating a custom dimension calculator myself to test its capabilities
    • Fixed height responsive images
    • Breakpoints with only a single dimension for old image format (JPG)
      • Breakpoints need to be separate from Sources
  • Revise if DimensionCalculator methods arguments can be simplified to only Breakpoint (as it already contains asset property for Asset)
  • Check placeholder generation string length
  • Recheck Fieldtype changes didn't bork controlpanel
  • Add mimetype too original format too
  • Check if getSources() isnt called too much
  • Check for any performance regression (tag and graphql)
  • Update README
  • Add UPGRADE.md?

Fix max widths config value or glide width param not being respected in some cases
Fix floating numbers for width and height values for Glide endpoint, they are not rounded integers
Add additional tests for dimension calculator and GraphQL
Clean up tests and output assets and content in tmp directory
…nal asset

As there can be custom dimension calculators, the dimensions on <img> tag will be custom.
Add more tests for dimension calculator.
…` in `Breakpoint`

Cache calls in `breakpoints()` and `sources()`
Was not very descriptive, was it?
Comment on lines +54 to +56
public function __set($name, $value): void
{
throw new Exception(sprintf('Cannot modify property %s', $name));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Read only properties in classes are available only starting with PHP 8.1. As we are exposing Breakpoint and Source classes now for DimensionCalculator, as such need to be aware what methods and properties are available.

}

return "(min-width: {$this->value}{$this->unit})";
}
$formats = collect(['avif', 'webp', 'original']);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So far this is how addon has worked: if you provided a JPG/PNG/WEBP etc. as source asset, then the default "srcSet" would Glide manipulate to the same format as the original asset was. This is default Glide behavior if you do not specify a format.

Responsive images addon config has option to toggle webp and avif formats, and technically you can change default quality amount for each format too.

The original format has been introduced here, and here's few key notes as to why:

  1. We have to deal with source image being any possible image format that Glide/Intervention supports, as such we would have to add format toggles for each image format. Having just one original seems simpler. Nowadays we really only care for webp and avif as their support becomes more robust as time goes.
  2. This original format cannot be exchanged to just jpg because in situations where source asset is a transparent image and that transparency needs to be maintained. Both WEBP and AVIF support transparency. If we force JPG, then transparency will become borked.

For now original is not publicly available to devs through config or params. But if you really want to disable original sources, then you can through DimensionCalculator. If we are introducing breaking changes, we could add original image format to the config too, but I decided against it for now as this PR is becoming too big and would be out of scope technically, trying to remain lean here.

public function toArray(): array
{
return [
'asset' => $this->asset,
'label' => $this->label,
'value' => $this->value,
'media' => $this->getMediaString(),
'minWidth' => $this->minWidth,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

value in my opinion felt too vague as property name, so I have renamed it to minWidth.

Copy link
Member

Choose a reason for hiding this comment

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

👍

Comment on lines +218 to +221
// Check if DimensionCalculator is instance of ResponsiveDimensionCalculator
// as ratio is only property applicable only for this DimensionCalculator
if (app(DimensionCalculator::class) instanceof ResponsiveDimensionCalculator) {
$data['ratio'] = app(DimensionCalculator::class)->breakpointRatio($this->asset, $this);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For legacy purposes, ratio seems only relevant if you are using the original ResponsiveDimensionCalculator.

'type' => GraphQL::int(),
'description' => 'The min-width of this breakpoint.',
],
'unit' => [
'widthUnit' => [
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Opinionated change, I just think this is more descriptive.

@@ -32,5 +32,10 @@ public function handle(): string
return $this->imageUrl();
}

public function getParams(): array
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I needed something publicly accessible for params for tests.

Comment on lines +32 to +44
public function getMimeType(): string|null
{
$mimeTypesBySetFormat = [
'webp' => 'image/webp',
'avif' => 'image/avif',
];

if (isset($mimeTypesBySetFormat[$this->format])) {
return $mimeTypesBySetFormat[$this->format];
}

return $this->breakpoint->asset->mimeType();
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This actually benefits for the original format, as previously the <source> tag for it did not contain mimetype at all, and browser has more difficult time without a mimetype specified. For example, if source asset was AVIF image, then original srcset could be unnecessarily loaded by the browser and may make the image broken, as AVIF image may not be supported by some browsers. I don't think browsers can guess mimetype from the provided URLs that Glide URL generator gives.

@ncla ncla marked this pull request as ready for review January 28, 2023 12:44
@ncla ncla requested a review from riasvdv January 28, 2023 12:45
Copy link
Member

@riasvdv riasvdv left a comment

Choose a reason for hiding this comment

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

Awesome!

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