Improved Image API #1956

supernova-at · 2019-11-04T15:38:04Z

Description

This PR improves the Image component API. Specifically, it:

No longer restricts breakpoints / image widths to three ("small", "medium", "large") sizes only
Renames resourceSizes / resourceSizeBreakpoints to widths and makes it a Map
Removes resourceWidth in favor of width (or the default entry in widths, if not present)
Removes resourceHeight in favor of height

Related Issue

Closes JIRA-90.

Acceptance

Verification Stakeholders

@zetlen @jimbo

Specification

Verification Steps

📝 This is just a code refactor. There should be no impact on the UI.

Ensure images work correctly throughout the application

Screenshots / Screen Captures (if appropriate)

Checklist

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

…Breakpoints

PWAStudioBot · 2019-11-04T15:41:58Z

	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).
📖	Associated JIRA tickets: JIRA-90.

Generated by 🚫 dangerJS against 533dfca

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

revanth0212 · 2019-11-04T16:03:34Z

@supernova-at there are prettier fails. Check on that once. I am not sure how did it miss the git commit hook when you pushed them.

revanth0212 · 2019-11-04T16:11:26Z

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

-        if (propResourceWidth) {
-            return propResourceWidth;
+        if (!widths) {
+            return undefined;


Can this not be any default value returned if widths is not provided?

It is odd that this would be undefined. I would assume that the components that care about this require it, right? We definitely don't want resourceImage throwing when it attempts to get widths[index] here.

Maybe instead of having the useImage talon do this work you could just pass the widths array to the components that use it and have them do this work. We're already talking about potentially splitting them anyways so that components can directly import resource or simple image rather than Image.

This is a little shortcut if the caller doesn't pass a width but does pass widths.

If they don't pass either one, we literally don't have any information to go off of - undefined seemed the best choice here. This will result in the img element rendered to the DOM to not have a width attribute and the network request to fetch this image won't include resize parameters.

I realized it wasn't preferring the passed-in width so I updated that in 4e798f7.

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

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

sirugh

Renaming is good 👍 for that.

However, I don't think an array is the right structure for maintaining the links between widths and breakpoints. I think a map fits this use case perfectly and would really simplify the logic in the sizes memo in useResourceImage.

const sizes = new Map();
sizes.set('default', 150)
sizes.set(100, 150);
sizes.set(200, 250);

const widths = [];
for (const [breakpoint, size] of sizes) {
  if (breakpoint !== 'default') {
    widths.push(`(max-width: ${breakpoint}px) ${size}px`);
  }
}

widths.push(`${sizes.get('default')}px`)
console.log(widths.join(', '))
// (max-width: 100px) 150px, (max-width: 200px) 250px, 150px

jimbo · 2019-11-05T18:24:00Z

However, I don't think an array is the right structure for maintaining the links between widths and breakpoints. I think a map fits this use case perfectly and would really simplify the logic in the sizes memo in useResourceImage.

Good suggestion. 👍

supernova-at · 2019-11-06T15:18:55Z

On it! 🖖

supernova-at · 2019-11-06T17:10:29Z

PR updated with new Map structure.

I named the prop widths instead of sizes to avoid confusion with the actual sizes attribute of an image (which we generate here but might be passed in by a caller).

jimbo

Good changes. 👍

jimbo · 2019-11-06T22:50:23Z

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

@@ -5,16 +5,12 @@ import { useCallback, useMemo, useState } from 'react';
 *
 * @param {function} props.onError callback for error of loading image
 * @param {function} props.onLoad callback for load of image
- * @param {Map}      props.resourceSizes image sizes used by the browser to select the image source. Supported keys are 'small', 'medium', and 'large'.
- * @param {number}   props.resourceWidth the intrinsic width of the image & the width to request for the fallback image for browsers that don't support srcset / sizes.
+ * @param {string}   props.unconstrainedSizeKey the key in props.widths for the unconstrained / default width.


This is pretty weird. Is there a reason why we can't just let "default" be the key here?

It is always "default", I just didn't want every file to have to know that.

I agree it is weird though. I'm not married to the idea.

The other way I thought of doing it was importing the key but then I ran into circular dependencies and Peregrine talons needing to import Venia UI files.

Maybe it could live on the useImage talon, I'll try that on for size.

Yep, way better: 14fca61.

The other way I thought of doing it was importing the key but then I ran into circular dependencies and Peregrine talons needing to import Venia UI files.

Right, the dependency should flow in one direction; we shouldn't have any situations where peregrine imports from venia-ui.

…pwa-studio into supernova/90_improved_image_api

supernova-at · 2019-11-07T15:33:18Z

PR Updated:

No longer threads the UNCONSTRAINED_SIZE_KEY
Simplifies most common use case: have a width but no widths. sizes will just use the given width as the unconstrained size. Ex: width=100 => sizes="100px".

jimbo · 2019-11-07T18:08:12Z

PR Updated:

* No longer threads the `UNCONSTRAINED_SIZE_KEY`

* Simplifies most common use case: have a `width` but no `widths`. `sizes` will just use the given `width` as the unconstrained size. Ex: `width=100 => sizes="100px"`.

Excellent. 💯

jimbo

Approved from my end. Will wait for @sirugh to approve as well.

supernova-at added 4 commits November 4, 2019 07:57

Image resourceSizes prop from Map to array

ff0ac89

Renames resourceSizes and resourceSizeBreakpoints to widths and width…

07aed8c

…Breakpoints

Renames resourceHeight to height

03c0438

Updates unit tests & stories

bac739b

PWAStudioBot added pkg:peregrine pkg:venia-ui labels Nov 4, 2019

supernova-at commented Nov 4, 2019

View reviewed changes

packages/peregrine/lib/talons/Image/useResourceImage.js Outdated Show resolved Hide resolved

m2-community-project bot assigned supernova-at Nov 4, 2019

m2-community-project bot added the Progress: review label Nov 4, 2019

m2-community-project bot removed the Progress: review label Nov 4, 2019

m2-community-project bot assigned sirugh Nov 4, 2019

m2-community-project bot added the Progress: review label Nov 4, 2019

revanth0212 reviewed Nov 4, 2019

View reviewed changes

m2-community-project bot assigned revanth0212 Nov 4, 2019

Runs prettier

0dcec43

supernova-at added the version: Major This changeset includes incompatible API changes and its release necessitates a Major version bump. label Nov 4, 2019

Merge branch 'develop' into supernova/90_improved_image_api

44965d2

sirugh requested changes Nov 5, 2019

View reviewed changes

m2-community-project bot added Progress: changes requested and removed Progress: review labels Nov 6, 2019

Updates breakpoints and widths to Map structure

fa254e2

supernova-at and others added 3 commits November 6, 2019 11:26

Uses width if present for resourceWidth

4e798f7

Properly threads width down to SimpleImage

432eadd

Merge branch 'develop' into supernova/90_improved_image_api

7fc9e18

jimbo reviewed Nov 6, 2019

View reviewed changes

m2-community-project bot assigned jimbo Nov 6, 2019

m2-community-project bot added Progress: review and removed Progress: changes requested labels Nov 6, 2019

supernova-at added 2 commits November 7, 2019 09:09

Updates unconstrained size key and simplifies single width use case

14fca61

Merge branch 'supernova/90_improved_image_api' of github.com:magento/…

3baeb69

…pwa-studio into supernova/90_improved_image_api

jimbo approved these changes Nov 7, 2019

View reviewed changes

sirugh approved these changes Nov 7, 2019

View reviewed changes

Merge branch 'develop' into supernova/90_improved_image_api

533dfca

dpatil-magento merged commit 872ab78 into develop Nov 12, 2019

dpatil-magento deleted the supernova/90_improved_image_api branch November 12, 2019 16:06

m2-community-project bot removed the Progress: review label Nov 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved Image API #1956

Improved Image API #1956

supernova-at commented Nov 4, 2019 •

edited

Loading

PWAStudioBot commented Nov 4, 2019 •

edited

Loading

revanth0212 commented Nov 4, 2019

revanth0212 Nov 4, 2019

sirugh Nov 5, 2019

supernova-at Nov 6, 2019

supernova-at Nov 6, 2019

sirugh left a comment •

edited

Loading

jimbo commented Nov 5, 2019

supernova-at commented Nov 6, 2019

supernova-at commented Nov 6, 2019

jimbo left a comment

jimbo Nov 6, 2019

supernova-at Nov 7, 2019 •

edited

Loading

supernova-at Nov 7, 2019 •

edited

Loading

supernova-at Nov 7, 2019

jimbo Nov 7, 2019

supernova-at commented Nov 7, 2019 •

edited

Loading

jimbo commented Nov 7, 2019

jimbo left a comment

Improved Image API #1956

Improved Image API #1956

Conversation

supernova-at commented Nov 4, 2019 • edited Loading

Description

Related Issue

Acceptance

Verification Stakeholders

Specification

Verification Steps

Screenshots / Screen Captures (if appropriate)

Checklist

PWAStudioBot commented Nov 4, 2019 • edited Loading

revanth0212 commented Nov 4, 2019

revanth0212 Nov 4, 2019

Choose a reason for hiding this comment

sirugh Nov 5, 2019

Choose a reason for hiding this comment

supernova-at Nov 6, 2019

Choose a reason for hiding this comment

supernova-at Nov 6, 2019

Choose a reason for hiding this comment

sirugh left a comment • edited Loading

Choose a reason for hiding this comment

jimbo commented Nov 5, 2019

supernova-at commented Nov 6, 2019

supernova-at commented Nov 6, 2019

jimbo left a comment

Choose a reason for hiding this comment

jimbo Nov 6, 2019

Choose a reason for hiding this comment

supernova-at Nov 7, 2019 • edited Loading

Choose a reason for hiding this comment

supernova-at Nov 7, 2019 • edited Loading

Choose a reason for hiding this comment

supernova-at Nov 7, 2019

Choose a reason for hiding this comment

jimbo Nov 7, 2019

Choose a reason for hiding this comment

supernova-at commented Nov 7, 2019 • edited Loading

jimbo commented Nov 7, 2019

jimbo left a comment

Choose a reason for hiding this comment

supernova-at commented Nov 4, 2019 •

edited

Loading

PWAStudioBot commented Nov 4, 2019 •

edited

Loading

sirugh left a comment •

edited

Loading

supernova-at Nov 7, 2019 •

edited

Loading

supernova-at Nov 7, 2019 •

edited

Loading

supernova-at commented Nov 7, 2019 •

edited

Loading