#2333 Add optional ratio for ressources #2372

larsroettig · 2020-04-30T21:43:21Z

Description

As a developer I'd love to re-use the <Image/> component for all custom components that render images. Unfortunately that's not ideal since images produced by the <Image/> component are always in a 4:5 ratio meaning that a banner image that needs to be 16:9 won't work properly.

Related Issue

Closes #2333.

Acceptance

I'd like to see a solution where the implementation decides what ratio the image needs to be instead of the internal image logic forcing it to use a set ratio. Something like this would be sufficient:
<Image ratio={4/3} />

Verification Steps

Go to Storybook
An Image using a Magento resource with resource constraints with a ratio
-> should display a 16 / Image

Screenshots / Screen Captures (if appropriate)

Checklist

I have updated the documentation accordingly, if necessary.
I have added tests to cover my changes, if necessary.

Jordaneisenburger · 2020-05-01T09:04:31Z

Sweet! I remember a short convo with Tommy saying he didn't like the explicit "ratio={1/2}" API. Not sure why, but maybe @tjwiebell could elaborate it a bit more ?

PWAStudioBot · 2020-05-02T01:17:23Z

	Messages
📖	Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.
📖	DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).

Generated by 🚫 dangerJS against 7e3e815

Jordaneisenburger · 2020-05-05T16:54:06Z

To really test this scenario you should change the story test case to

stories.add(
    'An Image using a Magento resource with resource constraints with ratio',
    () => {
        const widths = new Map()
            .set(320, 240)
            .set(768, 480)
            .set(1024, 960)
            .set(UNCONSTRAINED_SIZE_KEY, 1280);
        return (
            <div className={classes.container}>
                <Image
                    alt="An Image using a Magento resource with resource constraints"
                    classes={{root: classes.root}}
                    resource={getSrc19('480x270')}
                    ratio={16 / 9}
                    widths={widths}
                />
            </div>
        )
    }
);

Now what's going to happen is that the <Image/> is going to use the images.js util that has a "hardcoded" imageWidths map aswell. Which it's going to map against to find the best width that is closest to the intrinsicWidth. But this particular map also needs to be adjustable per image component since we want to use this component for all our images. So right now in our test case for the 768px breakpoint it's not going to load the most optimal image which would be 480 but rather load LARGE: 640, which is suboptimal.

@tjwiebell this is the issue I addressed in the meeting.

revanth0212

Hey, @larsroettig thanks for working on this. I have a couple of changes for you. Minor aesthetics nothing major.

packages/venia-ui/lib/util/images.js

packages/venia-ui/lib/components/Image/image.js

packages/peregrine/lib/talons/Image/useResourceImage.js

Co-authored-by: Revanth Kumar Annavarapu <35203638+revanth0212@users.noreply.github.com>

revanth0212 · 2020-05-20T16:54:19Z

Weird, all tests passed in local. But danger says tests have failed.

devops-pwa-codebuild · 2020-05-20T17:06:15Z

Performance Test Results

The following fails have been reported by WebpageTest. These numbers indicates a possible performance issue with the PR which requires further manual testing to validate.

https://pr-2372.pwa-venia.com : LH Performance Expected 0.85 Actual 0.58, LH Best Practices Expected 1 Actual 0.92
https://pr-2372.pwa-venia.com/venia-tops.html : LH Performance Expected 0.75 Actual 0.37, LH Best Practices Expected 1 Actual 0.92
https://pr-2372.pwa-venia.com/valeria-two-layer-tank.html : LH Performance Expected 0.8 Actual 0.5, LH Accessibility Expected 0.9 Actual 0.89, LH Best Practices Expected 1 Actual 0.92

tjwiebell

ResourceImage works perfect 👍 but need to account for some additional use cases. While stepping through, I also wanted to confirm an assumption I had that didn't quite work.

I assumed as an SI, if I was using a different aspect ratio for all of my images, I could change the value of DEFAULT_WIDTH_TO_HEIGHT_RATIO, and that would propagate throughout the app; but it didn't. Since @revanth0212 is the subject expert, I just wanted to confirm; if that constant isn't driving the aspect ratio of images in Venia, what is?

tjwiebell · 2020-05-21T14:35:40Z

packages/venia-ui/lib/components/Image/image.js

@@ -45,6 +46,7 @@ const Image = props => {
        type,
        width,
        widths,
+        ratio,


I would expect that ratio should also work with the SimpleImage component as well. I don't want to get into a huge rabbit hole re-factor, but the useImage talon seems like the best place to have this logic; if height isn't provided but ratio is, calculate height and drill that down.

I haven't thought of that use case. Is it possible? Neverthless, it is good to have a check. Done. Nice suggestion @tjwiebell. Fixed in 5536d61

tjwiebell · 2020-05-21T14:41:57Z

packages/venia-ui/lib/util/images.js

    if (!imageURL || !type) return '';

+    const imageRatio = ratio || DEFAULT_WIDTH_TO_HEIGHT_RATIO;


This API would need to change, so maybe drilling ratio is still best to keep things simple.

I have made sure to add default props for the ratio in the components that end up using this util.

tjwiebell · 2020-05-21T14:42:27Z

packages/venia-ui/lib/util/images.js

@@ -57,17 +57,18 @@ export const generateUrlFromContainerWidth = (
    );


Why isn't the line above also taking ratio into account?

Nice question. No reason not to have it. Fixed in b5126ab.

Note: I have not gone into the useProductImageCarousel talon, ProductImageCarousel component, and the components using this component to make the change of sending it as a prop. Felt out of scope for this PR. If someone wants to change this, they can send it as a prop and then as an argument to this function, if not the function uses the default ratio.

tjwiebell · 2020-05-21T14:48:40Z

packages/peregrine/lib/talons/Image/useResourceImage.js

@@ -21,16 +22,17 @@ export const useResourceImage = props => {
        resource,
        type,
        width,
-        widths
+        widths,
+        ratio
    } = props;

    const src = useMemo(() => {
        return generateUrl(resource, type)(width, height);


Ratio should also be accounted for here. Some browsers load src and srcset side by side, and could cause the container to be the wrong size (without the right ratio).

Because, after this commit 5536d61, the height sent to ResourceImage and then to useResourceImage is checked against the ratio. I don't think we will need this any more.

Jordaneisenburger · 2020-05-22T10:20:33Z

To really test this scenario you should change the story test case to
stories.add(
    'An Image using a Magento resource with resource constraints with ratio',
    () => {
        const widths = new Map()
            .set(320, 240)
            .set(768, 480)
            .set(1024, 960)
            .set(UNCONSTRAINED_SIZE_KEY, 1280);
        return (
            <div className={classes.container}>
                <Image
                    alt="An Image using a Magento resource with resource constraints"
                    classes={{root: classes.root}}
                    resource={getSrc19('480x270')}
                    ratio={16 / 9}
                    widths={widths}
                />
            </div>
        )
    }
);
Now what's going to happen is that the <Image/> is going to use the images.js util that has a "hardcoded" imageWidths map aswell. Which it's going to map against to find the best width that is closest to the intrinsicWidth. But this particular map also needs to be adjustable per image component since we want to use this component for all our images. So right now in our test case for the 768px breakpoint it's not going to load the most optimal image which would be 480 but rather load LARGE: 640, which is suboptimal.

@tjwiebell this is the issue I addressed in the meeting.

Hey guys, just wanna make sure the issue I point out isn't forgotten. Maybe I misinterpret the image helper, if so you can ignore this comment.

revanth0212 · 2020-05-22T21:25:13Z

Hey guys, just wanna make sure the issue I point out isn't forgotten. Maybe I misinterpret the image helper, if so you can ignore this comment.

Added the story in 791c526

@Jordaneisenburger is this the expected result?

tjwiebell

Looks good 👍 we should still confirm with @Jordaneisenburger that we've solved that edge case with the imageWidths Map.

…the browser.

revanth0212 · 2020-05-27T15:47:29Z

packages/venia-ui/lib/components/Image/image.js

@@ -108,7 +108,7 @@ const Image = props => {
                alt={alt}
                classes={classes}
                displayPlaceholder={displayPlaceholder}
-                height={talonResourceHeight}
+                height={height}


Not setting the calculate talon height on the image because this will override the srcset on the img tag. srcset works best with width. By providing height we are making the container have a fixed height and because of that, the image stretches. In this example, the width of the image was 640px so using the ratio 4/5 the height turned out to be 800px. But the srcset calculated width by the browser is 375px but since we are setting 800 as height, the image was being stretched.

Before:

After:

…-studio into 2333-image-ratio

dpatil-magento · 2020-05-28T16:55:11Z

Regression looks good on image load w/o fastly image opt auto/onboard/backend. Good to merge once review approved and @Jordaneisenburger confirms on the fix.

#2333 Add optional ratio for ressources

996c72a

larsroettig requested review from supernova-at and revanth0212 April 30, 2020 21:43

magento-engcom-team added Partner: TechDivision partners-contribution labels Apr 30, 2020

dpatil-magento added the CI_APPROVED This PR should be included in the CI process. label May 1, 2020

PWAStudioBot added pkg:peregrine pkg:venia-ui labels May 2, 2020

Merge branch 'develop' into 2333-image-ratio

1a5c7db

larsroettig requested a review from jimbo May 8, 2020 16:51

Merge branch 'develop' into 2333-image-ratio

afaff11

revanth0212 requested changes May 11, 2020

View reviewed changes

m2-community-project bot assigned revanth0212 May 11, 2020

m2-community-project bot added the Progress: changes requested label May 11, 2020

Lars Roettig and others added 2 commits May 11, 2020 18:37

Apply suggestions from code review

e0e70e5

Co-authored-by: Revanth Kumar Annavarapu <35203638+revanth0212@users.noreply.github.com>

PR changes.

e84564e

revanth0212 and others added 2 commits May 20, 2020 11:58

Prettier changes.

ddbf88c

Merge branch 'develop' into 2333-image-ratio

15c3d67

revanth0212 added the version: Minor This changeset includes functionality added in a backwards compatible manner. label May 20, 2020

tjwiebell suggested changes May 21, 2020

View reviewed changes

m2-community-project bot assigned tjwiebell May 21, 2020

Deriving height if not provided in props using ratio.

5536d61

revanth0212 and others added 3 commits May 22, 2020 16:16

Merge branch 'develop' into 2333-image-ratio

42781c4

Added new story to images story book.

791c526

Fixed styleguide build failure.

762f1d9

Merge branch 'develop' into 2333-image-ratio

c982c5d

tjwiebell closed this May 26, 2020

tjwiebell reopened this May 26, 2020

m2-community-project bot added Progress: review and removed Progress: changes requested labels May 26, 2020

tjwiebell previously approved these changes May 26, 2020

View reviewed changes

dpatil-magento and others added 2 commits May 26, 2020 13:27

Merge branch 'develop' into 2333-image-ratio

4e4b4e0

Not setting height on image, it will override the src calculation by …

88cdbd5

…the browser.

revanth0212 dismissed tjwiebell’s stale review via 88cdbd5 May 27, 2020 15:43

revanth0212 reviewed May 27, 2020

View reviewed changes

dpatil-magento and others added 4 commits May 27, 2020 14:01

Merge branch 'develop' into 2333-image-ratio

d1a0b39

Snapshot updates.

eae645d

Merge branch '2333-image-ratio' of https://github.com/larsroettig/pwa…

7aa036e

…-studio into 2333-image-ratio

Merge branch 'develop' into 2333-image-ratio

569c2a8

tjwiebell approved these changes May 28, 2020

View reviewed changes

Merge branch 'develop' into 2333-image-ratio

7e3e815

dpatil-magento merged commit d1a9fa3 into magento:develop Jun 1, 2020

m2-community-project bot removed the Progress: review label Jun 1, 2020

sirugh added Priority: P3 Severity: S2 labels Dec 18, 2020

m2-community-project bot added Progress: done labels Dec 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#2333 Add optional ratio for ressources #2372

#2333 Add optional ratio for ressources #2372

larsroettig commented Apr 30, 2020 •

edited by tjwiebell

Loading

Jordaneisenburger commented May 1, 2020

PWAStudioBot commented May 2, 2020 •

edited

Loading

Jordaneisenburger commented May 5, 2020 •

edited

Loading

revanth0212 left a comment

revanth0212 commented May 20, 2020

devops-pwa-codebuild commented May 20, 2020 •

edited

Loading

tjwiebell left a comment

tjwiebell May 21, 2020

revanth0212 May 22, 2020

tjwiebell May 21, 2020

revanth0212 May 22, 2020

tjwiebell May 21, 2020

revanth0212 May 22, 2020

tjwiebell May 21, 2020

revanth0212 May 22, 2020

Jordaneisenburger commented May 22, 2020

revanth0212 commented May 22, 2020

tjwiebell left a comment

revanth0212 May 27, 2020 •

edited

Loading

dpatil-magento commented May 28, 2020

@@ @@ -45,6 +46,7 @@ const Image = props => { @@
                       type,
                       width,
                       widths,
+                      ratio,

		if (!imageURL \|\| !type) return '';

		const imageRatio = ratio \|\| DEFAULT_WIDTH_TO_HEIGHT_RATIO;

		@@ -57,17 +57,18 @@ export const generateUrlFromContainerWidth = (
		);

#2333 Add optional ratio for ressources #2372

#2333 Add optional ratio for ressources #2372

Conversation

larsroettig commented Apr 30, 2020 • edited by tjwiebell Loading

Description

Related Issue

Acceptance

Verification Steps

Screenshots / Screen Captures (if appropriate)

Checklist

Jordaneisenburger commented May 1, 2020

PWAStudioBot commented May 2, 2020 • edited Loading

Jordaneisenburger commented May 5, 2020 • edited Loading

revanth0212 left a comment

Choose a reason for hiding this comment

revanth0212 commented May 20, 2020

devops-pwa-codebuild commented May 20, 2020 • edited Loading

tjwiebell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jordaneisenburger commented May 22, 2020

revanth0212 commented May 22, 2020

tjwiebell left a comment

Choose a reason for hiding this comment

revanth0212 May 27, 2020 • edited Loading

Choose a reason for hiding this comment

dpatil-magento commented May 28, 2020

larsroettig commented Apr 30, 2020 •

edited by tjwiebell

Loading

PWAStudioBot commented May 2, 2020 •

edited

Loading

Jordaneisenburger commented May 5, 2020 •

edited

Loading

devops-pwa-codebuild commented May 20, 2020 •

edited

Loading

revanth0212 May 27, 2020 •

edited

Loading