Image and video optimizations #1769

jonkafton · 2024-10-28T21:13:11Z

What are the relevant tickets?

Closes https://github.com/mitodl/hq/issues/5857

Upgrades to Next.js v15

Description (What does it do?)

Replaces <img> tags with <Image> components where it makes sense to.
- There are places where it doesn't make sense to, such as where we have containers that would prevent layout shift and where we are already able to provide the image as a static import (Next.js set the src string to its image optimization endpoint), e.g. the PersonalizeSection image.
- Static imports for images in repo (import src is a path to the Next.js image optimization endpoint). This is used for both <img> tags and CSS background-image.
- Provides a utility that produces a CSS image-set() declaration for a statically imported image. These are links for the image from the optimization endpoint at resolutions for 1x and 2x screen pixel densities, see https://developer.mozilla.org/en-US/docs/Web/CSS/image/image-set. Applied to the homepage personalize section background. These can also be used on <img> tags (srcset attribute). We should consider applying elsewhere, particularly for large images, though also should use with caution as it may not be desirable to render high resolution images for example for backgrounds that contain little detail.
- The resource drawer images are now optimized by Next.js with noticeable improvement to quality:
  - Before:
  - After:
Resource drawer YouTube videos are displayed using a simple iframe (YouTube's embed html is similar). Non-YouTube videos fall back to Embedly, though all of our videos are YouTube at present. This also removes some padding added by Embedly and sizes the videos to match our aspect ratios and applies the border radius.
- Before:
- After:
- Updates ol-components/Logo to support unit logos and distinguish between units and platforms so it can be used on the unit channel pages. We can't import from the ./public/images folder to use image optimization as we have the rule that ol-components are leaf imports, though there is no value for SVGs.
- Clean up repetitions of the Embedly key / move into EmbedlyCard.

How can this be tested?

Check that images and logos display correctly at mobile and desktop. Particular attention to:

Resource drawer images and video.
Homepage personalize section background.
Nav drawer icons.
About page main image.
Topics listing page banner background.
Channel pages banner background.
Units listing page card logos.

…onent

…sonalize section

… be wrapped error)

frontends/main/src/app-pages/HomePage/HomePage.tsx

jonkafton · 2024-10-29T18:08:27Z

frontends/main/src/app-pages/ChannelPage/UnitChannelTemplate.tsx

+                    <DesktopOnly>
+                      <PlatformLogoInverted
+                        unitCode={name as OfferedByEnum}
+                        height={50}
+                      />
+                    </DesktopOnly>
+                    <MobileOnly>
+                      <PlatformLogoInverted
+                        unitCode={name as OfferedByEnum}
+                        height={40}
+                      />
+                    </MobileOnly>


Here we produce for each side of the width breakpoint and use the css media query to hide one. This is wordy, though needed for varying the height with only CSS.

Where the images are in the repo we can generally pull them in as static imports, so don't need to specify the dimensions, though in this case the ol-components PlatformLogo is not allowed to import from outside its workspace.

Further, the width and height on <Image> components do not size the image itself - it is just to block space to prevent layout shift, which can be done with CSS. Here though the logos are of different sizes so we need to hardcode the dimensions (here providing fixed height with width calculated in PlatformLogo).

Where the images are in the repo we can generally pull them in as static imports,

That's why Logo.tsx specifies the dimensions, but we shouldn't have to here, right? I think you can achieve the same effect just with

const PlatformLogoInverted = styled(PlatformLogo)(({ theme }) => ({ filter: "saturate(0%) invert(100%)", height: 50, maxWidth: "100%", [theme.breakpoints.down("md")]: { height: 40, width: "auto", }, }))

Probably a separate Issue: Doesn't seem great to be specifying aspect ratios / widths in Logo.tsx. Maybe we should just move the enum'd labels to ol-components. I guess we probably need to static string paths for email templates or something. (Taking a quick look.... maybe we don't use them anyway in the backend.)

jonkafton · 2024-10-29T18:23:34Z

frontends/main/src/app-pages/DepartmentListingPage/DepartmentListingPage.tsx

            current="Departments"
          />
        }
+        backgroundUrl={backgroundSteps.src}


The <Banner> component has a default image path (background_steps.jpg), but as this is just a path string, the image is loaded directly as opposed to from the image optimization endpoint. Static imports get the image optimzation endpoint, but as a rule the <Banner> component cannot import from outside ol-components.

Assuming we publish ol-components as a separate package, and I guess this could wait until then, we should do one of the following:

Not include Banner in ol-components, move it to main/src/components

Remove the string-based default background image from Banner Doesn't make sense unless I guess it's an absolute / fully qualified URL to some server somewhere.

Move the default background image to ol-components.

I would probably lean toward (1). Our Banner component has lots of props enabling it to be used for (A) simple cases like /departments, /topics, etc, and (B) complex cases like the unit channel pages. I'd be happy to include (A) in the published design system, but (B) seems too Learn-specific. I think the two cases are different components in Figma, too.

frontends/main/src/app-pages/HomePage/HomePage.tsx

…) should be wrapped error)" This reverts commit b96fb61.

This reverts commit 18fee78.

This reverts commit 20f0f05.

ChristopherChudzicki

This is working great. I left a few comments, some of which we should probably make sense to do now (remove comments, maybe use backgroundSrcSetCSS more, add fallbak to backgroundSrcSetCSS?).

ChristopherChudzicki · 2024-10-30T14:34:35Z

frontends/main/src/app-pages/AboutPage/AboutPage.tsx

+          <SubHeaderImageContainer>
+            <SubHeaderImage
+              src={domeImage}
+              alt="A view of the entablature of MIT's Great Dome"


today i learn...

ChristopherChudzicki · 2024-10-30T15:26:14Z

frontends/main/src/app-pages/ChannelPage/UnitChannelTemplate.tsx

+                    <DesktopOnly>
+                      <PlatformLogoInverted
+                        unitCode={name as OfferedByEnum}
+                        height={50}
+                      />
+                    </DesktopOnly>
+                    <MobileOnly>
+                      <PlatformLogoInverted
+                        unitCode={name as OfferedByEnum}
+                        height={40}
+                      />
+                    </MobileOnly>


Where the images are in the repo we can generally pull them in as static imports,

That's why Logo.tsx specifies the dimensions, but we shouldn't have to here, right? I think you can achieve the same effect just with

const PlatformLogoInverted = styled(PlatformLogo)(({ theme }) => ({ filter: "saturate(0%) invert(100%)", height: 50, maxWidth: "100%", [theme.breakpoints.down("md")]: { height: 40, width: "auto", }, }))

Probably a separate Issue: Doesn't seem great to be specifying aspect ratios / widths in Logo.tsx. Maybe we should just move the enum'd labels to ol-components. I guess we probably need to static string paths for email templates or something. (Taking a quick look.... maybe we don't use them anyway in the backend.)

ChristopherChudzicki · 2024-10-30T15:36:26Z

frontends/main/src/app-pages/DepartmentListingPage/DepartmentListingPage.tsx

            current="Departments"
          />
        }
+        backgroundUrl={backgroundSteps.src}


Assuming we publish ol-components as a separate package, and I guess this could wait until then, we should do one of the following:

Not include Banner in ol-components, move it to main/src/components

Remove the string-based default background image from Banner Doesn't make sense unless I guess it's an absolute / fully qualified URL to some server somewhere.

Move the default background image to ol-components.

I would probably lean toward (1). Our Banner component has lots of props enabling it to be used for (A) simple cases like /departments, /topics, etc, and (B) complex cases like the unit channel pages. I'd be happy to include (A) in the published design system, but (B) seems too Learn-specific. I think the two cases are different components in Figma, too.

ChristopherChudzicki · 2024-10-30T15:59:06Z

frontends/main/src/app-pages/TopicsListingPage/TopicsListingPage.tsx

        }
        title="Browse by Topic"
        header="Select a topic below to explore relevant learning resources across all Academic and Professional units."
+        backgroundUrl={backgroundSteps.src}


Any reason not to use backgroundSrcSetCSS here?

For this one the getImageProps() only produces the single 1x image, so using srcset has no impact.

Also, the backgroundUrl prop is used in the <Banner> css like linear-gradient(rgba(0 0 0 / ${backgroundDim}%), rgba(0 0 0 / ${backgroundDim}%)), url('${backgroundUrl}'), so we needed some extra gymnastics:

const backgroundUrlFn = backgroundUrl.startsWith("image-set(") ? backgroundUrl : `url('${backgroundUrl}')`

Works fine though.

frontends/main/src/app-pages/UnitsListingPage/UnitCard.tsx

ChristopherChudzicki · 2024-10-30T16:21:41Z

frontends/ol-utilities/package.json

    "iso-639-1": "^3.1.0",
    "lodash": "^4.17.21",
    "moment": "^2.29.4",
+    "next": "^14.2.15",


Doesn't really matter, but I'd make this a peer-dep along with react.

ChristopherChudzicki · 2024-10-30T16:23:18Z

frontends/ol-utilities/src/images/backgroundImages.ts

+    })
+    .join(", ")
+
+  return imageSet ? `image-set(${imageSet})` : ""


I don't understand when props.srcSet, and hence imageSet would be undefined. But I guess it can be?

Should this fall back to url(`${props.src}`)?

props.srcSet is typed string | undefined, though I'm not sure how it could be. Either way the backround image would be broken, so have removed the ternary.

ChristopherChudzicki · 2024-10-30T16:25:26Z

frontends/ol-components/src/components/Logo/Logo.tsx


 const DEFAULT_WIDTH = 200

 export const PlatformLogo: React.FC<{


Should we have separate PlatformLogo and UnitLogo components?

ChristopherChudzicki

👍

ChristopherChudzicki

👍

jonkafton added 14 commits October 24, 2024 22:26

Use Next.js Image component in the resource drawer

1687bfb

Merge branch 'main' into jk/5857-image-optimizations

28f4207

Differentiate NavItem props from config type

a3be265

Replace about page image

56e3dc5

Next.js images for channel page logos. Unit image config in Logo comp…

2f1f01f

…onent

Remove NavDrawer image paths (not used). Fix Storybook page

0fbacef

Upgrade to Next.js v15. Page params are now async

20f0f05

Upgrade @mui/material-nextjs for Next.js v15

18fee78

Utility for CSS background image-set() strings. Apply to homepage per…

3164d75

…sonalize section

Pass background static import src to banner

8dc2db9

Use Logo component for unit cards

92e8cd4

Merge branch 'main' into jk/5857-image-optimizations

ec40724

Suspense boundaries needed around carousels (useSearchParams() should…

b96fb61

… be wrapped error)

Type fix

f4d8056

jonkafton commented Oct 29, 2024

View reviewed changes

frontends/main/src/app-pages/HomePage/HomePage.tsx Outdated Show resolved Hide resolved

Display YouTube videos with simple iframe

4e4af0a

jonkafton changed the title ~~Image optimizations~~ Image and video optimizations Oct 29, 2024

jonkafton commented Oct 29, 2024

View reviewed changes

frontends/main/src/app-pages/HomePage/HomePage.tsx Outdated Show resolved Hide resolved

jonkafton added 7 commits October 29, 2024 20:05

16:9 aspect ratio for videos

f5e916f

Revert "Suspense boundaries needed around carousels (useSearchParams(…

d66ab6e

…) should be wrapped error)" This reverts commit b96fb61.

Revert "Upgrade @mui/material-nextjs for Next.js v15"

f97d90f

This reverts commit 18fee78.

Revert "Upgrade to Next.js v15. Page params are now async"

eb5b317

This reverts commit 20f0f05.

Update test for unit logo

00ef518

Add Nextjs dependency in ol-utilities

8bb8dcb

Merge branch 'main' into jk/5857-image-optimizations

cb37c04

ChristopherChudzicki self-assigned this Oct 30, 2024

ChristopherChudzicki requested changes Oct 30, 2024

View reviewed changes

Remove comments

d75212a

jonkafton added 6 commits October 30, 2024 19:46

Remove unnecessary truthy check

0ce9cf9

Separate Unit/Platform Logo

e38f0d9

Move to peer dependency

1086dde

Bckground src set for topic banner

36a3982

Background src set on other banner backgrounds

c868b00

Remove redundant display breakpoints

9386f27

ChristopherChudzicki approved these changes Oct 31, 2024

View reviewed changes

jonkafton added 4 commits October 31, 2024 19:16

Merge branch 'main' into jk/5857-image-optimizations

6ed1589

Apply changes to LearningResourceExpandedV2

9fc386a

Update test

97ad4b8

Merge branch 'main' into jk/5857-image-optimizations

186b748

ChristopherChudzicki approved these changes Nov 1, 2024

View reviewed changes

jonkafton merged commit 5951883 into main Nov 1, 2024
11 checks passed

jonkafton deleted the jk/5857-image-optimizations branch November 1, 2024 19:20

This was referenced Nov 5, 2024

Release 0.25.0 #1791

Closed

Release 0.24.2 #1794

Closed

Release 0.24.2 #1796

Merged


		const DEFAULT_WIDTH = 200

		export const PlatformLogo: React.FC<{

Image and video optimizations #1769

Image and video optimizations #1769

Uh oh!

Conversation

jonkafton commented Oct 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What are the relevant tickets?

Description (What does it do?)

How can this be tested?

Uh oh!

Uh oh!

jonkafton Oct 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ChristopherChudzicki 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

ChristopherChudzicki left a comment

Choose a reason for hiding this comment

Uh oh!

ChristopherChudzicki left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jonkafton commented Oct 28, 2024 •

edited

Loading

jonkafton Oct 29, 2024 •

edited

Loading