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

updated canvas usage #490

Merged
merged 2 commits into from
Nov 9, 2021
Merged

updated canvas usage #490

merged 2 commits into from
Nov 9, 2021

Conversation

SeanWahoo
Copy link
Contributor

latest canvas parameter requires a comma to be used, not a colon.

latest canvas parameter requires a comma to be used, not a colon.
@smaeda-ks
Copy link
Contributor

Hi @golferejoe16, could you elaborate on this change more?

https://developer.fastly.com/reference/io/canvas/

SIZE can be:
{width},{height}
{width-ratio}:{height-ratio}

ratio value needs :

@SeanWahoo
Copy link
Contributor Author

Sure, the code is not using ratios, it's using static widths and heights, so a comma is correct. The canvas should create an image where the original image is placed on a canvas of size width x height, not an image with a ratio of width:height. See this example where the image should be placed on a canvas of 615px x 615px. The original colon based solution will create a small square 300x300 image, the corrected comma based solution fixes this. Example:
(incorrect) https://www.wahoofitness.com/media/catalog/product/w/a/wahoo-logo-t-shirt-3.png?optimize=medium&fit=bounds&height=615&width=615&canvas=615:615
(correct) https://www.wahoofitness.com/media/catalog/product/w/a/wahoo-logo-t-shirt-3.png?optimize=medium&fit=bounds&height=615&width=615&canvas=615,615

@smaeda-ks
Copy link
Contributor

Thank you @golferejoe16
We're evaluating this change internally. cc: @dpotkoc

@smaeda-ks smaeda-ks merged commit e5dbfd8 into fastly:master Nov 9, 2021
MickaelDatadome pushed a commit to DataDome/fastly-magento2 that referenced this pull request Oct 5, 2023
* updated canvas usage

latest canvas parameter requires a comma to be used, not a colon.

* remove comment

Co-authored-by: Shohei Maeda <[email protected]>
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