Image rework #1271

frehner · 2022-05-13T20:25:02Z

Description

Closes #1223 and massively reworks the internal implementation of the Image component

Removes useImageUrl in favor of just calling the function shopifyImageLoader - it's actually probably faster / better, and it can also be run on the client and the server, unlike useImageUrl which can only be run on the client.

imageLoader now takes in an object instead of positional params, which makes it easy to only use what you care to use and skip the rest.

The TypeScript experience is improved; if you're using the data prop, you're using a SFAPI source and so the loader and loaderOptions will be typed for that. If you're using the src prop, we use a generic to infer the props you pass in to loaderOptions and apply that to the loader param. Also, when using src, width and height are required props.

Hopefully cleaned up some side effects in the Image tests that weren't being cleaned up before, but I'm still running into ts-jest issues that I can't figure out.

<Video/> props options turns into imagePreviewOptions to be more explicit what the options are for.

addImageSizeParametersToUrl function refactored to just use URL's .searchParams.append to make the logic much easier.

Despite the recent change to add the priority prop in #1245 , I decided it didn't make sense to essentially rename what is already a standardized HTML attribute, so I removed priority and changed the logic to make loading the source of truth.

Additional context

Todos before this is ready:

Update docs to match the above changes
Add a changelog and migration guide
Figure out why tf ts-jest is having issues parsing some tests

Before submitting the PR, please make sure you do the following:

Read the Contributing Guidelines
Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123)
Update docs in this repository according to your change
Run yarn changeset add if this PR cause a version bump based on Keep a Changelog and adheres to Semantic Versioning

frehner · 2022-05-13T21:07:59Z

jest.config.ts

-        lib: ['ESNext', 'DOM'],
-        target: 'es6',
-      },
+      tsconfig: './packages/hydrogen/tsconfig.json',


This fixes the weird ts-jest issues but I don't know if it has other side effects or not??

We shall see!

mcvinci

Thanks so much for this, @frehner! I added a few suggestions throughout to align the code comments with the new descriptions in the reference doc.

docs/components/primitive/image.md

mcvinci · 2022-05-14T05:52:46Z

docs/components/primitive/image.md

-| height         | <code>height</code>                              | The integer value for the height of the image. This is a required prop when `src` is present.                                                                                                        |
-| loader?        | <code>(props: ImageLoaderOptions): string</code> | A custom function that generates the image URL. Parameters passed into this function includes `src` and an `options` object that contains the provided `width`, `height` and `loaderOptions` values. |
-| loaderOptions? | <code>ImageLoaderOptions['options']</code>       | An object of `loader` function options. For example, if the `loader` function requires a `scale` option, then the value can be a property of the `loaderOptions` object (for example, `{scale: 2}`). |
-| priority? | <code>boolean</code>       | Whether the image will be immediately loaded. Defaults to `false`. This prop should be used only when the image is visible above the fold. For more information, refer to the [Image Embed element's loading attribute](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/img#attr-loading).   |


Should priority? be retained?

Nope it's gone

mcvinci · 2022-05-14T05:53:23Z

docs/components/primitive/image.md

 | `height` | A string of the pixel height (for example, `100px`) or `original` for the original height of the image. |
 | `crop`   | Valid values: `top`, `bottom`, `left`, `right`, or `center`.                                            |
 | `scale`  | Valid values: 2 or 3.                                                                                   |
-| `format` | Valid values: `jpg` or `pjpg`.                                                                          |


Just double-checking! Is this no longer an option?

packages/hydrogen/src/components/Video/Video.tsx

packages/hydrogen/src/components/Image/Image.tsx

Co-authored-by: Michelle Vinci <[email protected]>

docs/components/primitive/image.md

Co-authored-by: Michelle Vinci <[email protected]>

jplhomer

Terrific work, Anthony!

jplhomer · 2022-05-16T15:09:34Z

packages/hydrogen/src/components/Image/tests/Image.test.tsx


-    it('renders an `img` element without `width` and `height` attributes when invalid dimensions are provided', () => {
+    // eslint-disable-next-line jest/no-focused-tests
+    it.only('renders an `img` element without `width` and `height` attributes when invalid dimensions are provided', () => {


heads-up: looks like we need to remove it.only here

Whoops nice catch!

jplhomer · 2022-05-16T15:10:24Z

packages/hydrogen/src/utilities/image_size.ts

+  width && newUrl.searchParams.append('width', width.toString());
+  height && newUrl.searchParams.append('height', height.toString());
+  crop && newUrl.searchParams.append('crop', crop);
+  scale && newUrl.searchParams.append('scale', scale.toString());


Co-authored-by: Michelle Vinci <[email protected]>

Also cleaned up other tests from outputting to the console. Updated migration docs a tiny bit

frehner · 2022-05-16T16:45:38Z

@mcvinci just did a small documentation update, you can more easily see the changes I made by looking at this specific commit

Fixed the tests, and also mocked out console.log / console.error stuff that was polluting our test output. Once tests pass, I'll merge.

frehner added 6 commits May 12, 2022 09:23

saving progress

9ee6f79

saving progress

9fcb2eb

Change from error to warning

eef08d8

Fix image width types

4c89ec6

additional loader return value check

353b16c

Merge branch 'v1.x-2022-07' into image-rework

70ad54b

github-actions bot deployed to preview May 13, 2022 20:30 View deployment

Maybe fix ts-jest?

f8e4994

github-actions bot deployed to preview May 13, 2022 21:01 View deployment

frehner commented May 13, 2022

View reviewed changes

Add changelog

2d2b6e3

github-actions bot deployed to preview May 13, 2022 21:48 View deployment

documentation update

c6cd3a2

frehner marked this pull request as ready for review May 13, 2022 22:30

frehner requested a review from a team May 13, 2022 22:31

github-actions bot deployed to preview May 13, 2022 22:32 View deployment

mcvinci approved these changes May 14, 2022

View reviewed changes

frehner and others added 4 commits May 14, 2022 10:50

Update docs/components/primitive/image.md

0e5ccc5

Co-authored-by: Michelle Vinci <[email protected]>

Update docs/components/primitive/image.md

cc6bb5b

Co-authored-by: Michelle Vinci <[email protected]>

Update packages/hydrogen/src/components/Video/Video.tsx

a98954a

Co-authored-by: Michelle Vinci <[email protected]>

Update packages/hydrogen/src/components/Image/Image.tsx

46e8871

Co-authored-by: Michelle Vinci <[email protected]>

github-actions bot deployed to preview May 14, 2022 16:52 View deployment

Update packages/hydrogen/src/components/Image/Image.tsx

341f739

Co-authored-by: Michelle Vinci <[email protected]>

github-actions bot deployed to preview May 14, 2022 16:53 View deployment

frehner and others added 4 commits May 14, 2022 10:54

Update packages/hydrogen/src/components/Image/Image.tsx

c0ffabb

Co-authored-by: Michelle Vinci <[email protected]>

Update packages/hydrogen/src/components/Image/Image.tsx

b46c294

Co-authored-by: Michelle Vinci <[email protected]>

Update packages/hydrogen/src/components/Image/Image.tsx

ef442aa

Co-authored-by: Michelle Vinci <[email protected]>

Update packages/hydrogen/src/components/Image/Image.tsx

2409043

Co-authored-by: Michelle Vinci <[email protected]>

github-actions bot deployed to preview May 14, 2022 16:56 View deployment

github-actions bot deployed to preview May 14, 2022 16:57 View deployment

mcvinci reviewed May 14, 2022

View reviewed changes

docs/components/primitive/image.md Outdated Show resolved Hide resolved

mcvinci reviewed May 14, 2022

View reviewed changes

docs/components/primitive/image.md Outdated Show resolved Hide resolved

frehner and others added 3 commits May 14, 2022 15:27

Update packages/hydrogen/src/components/Image/Image.tsx

37af44d

Co-authored-by: Michelle Vinci <[email protected]>

Update packages/hydrogen/src/components/Image/Image.tsx

c0b2d20

Co-authored-by: Michelle Vinci <[email protected]>

apply prettier

457b812

github-actions bot deployed to preview May 14, 2022 21:29 View deployment

github-actions bot deployed to preview May 14, 2022 21:30 View deployment

jplhomer approved these changes May 16, 2022

View reviewed changes

frehner and others added 2 commits May 16, 2022 09:21

Update docs/components/primitive/image.md

2b7073c

Co-authored-by: Michelle Vinci <[email protected]>

Update docs/components/primitive/image.md

48ec9e6

Co-authored-by: Michelle Vinci <[email protected]>

github-actions bot deployed to preview May 16, 2022 15:23 View deployment

All Image tests running

8003184

Also cleaned up other tests from outputting to the console. Updated migration docs a tiny bit

github-actions bot deployed to preview May 16, 2022 16:42 View deployment

Merge branch 'v1.x-2022-07' into image-rework

baec07b

github-actions bot deployed to preview May 16, 2022 16:47 View deployment

frehner mentioned this pull request May 16, 2022

Support optional priority prop on Image component #1245

Merged

4 tasks

frehner merged commit 9d0359b into v1.x-2022-07 May 16, 2022

frehner deleted the image-rework branch May 16, 2022 17:04

github-actions bot mentioned this pull request May 16, 2022

[ci] release v1.x-2022-07 #1274

Merged

mcvinci mentioned this pull request May 25, 2022

[Hydrogen docs]: Fix formatting of Image props #1344

Merged

Image rework #1271

Image rework #1271

Uh oh!

Conversation

frehner commented May 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Additional context

Before submitting the PR, please make sure you do the following:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcvinci left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jplhomer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frehner commented May 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

frehner commented May 13, 2022 •

edited

Loading