Skip to content

Image and video optimizations #1769

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.

Sign up for GitHub

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

Merged
jonkafton merged 33 commits into from
Nov 1, 2024
Merged

Image and video optimizations #1769

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
1687bfb
Use Next.js Image component in the resource drawer
jonkafton Oct 24, 2024
28f4207
Merge branch 'main' into jk/5857-image-optimizations
jonkafton Oct 25, 2024
a3be265
Differentiate NavItem props from config type
jonkafton Oct 25, 2024
56e3dc5
Replace about page image
jonkafton Oct 25, 2024
2f1f01f
Next.js images for channel page logos. Unit image config in Logo comp…
jonkafton Oct 25, 2024
0fbacef
Remove NavDrawer image paths (not used). Fix Storybook page
jonkafton Oct 28, 2024
20f0f05
Upgrade to Next.js v15. Page params are now async
jonkafton Oct 28, 2024
18fee78
Upgrade @mui/material-nextjs for Next.js v15
jonkafton Oct 28, 2024
3164d75
Utility for CSS background image-set() strings. Apply to homepage per…
jonkafton Oct 28, 2024
8dc2db9
Pass background static import src to banner
jonkafton Oct 28, 2024
92e8cd4
Use Logo component for unit cards
jonkafton Oct 28, 2024
ec40724
Merge branch 'main' into jk/5857-image-optimizations
jonkafton Oct 28, 2024
b96fb61
Suspense boundaries needed around carousels (useSearchParams() should…
jonkafton Oct 28, 2024
f4d8056
Type fix
jonkafton Oct 29, 2024
4e4af0a
Display YouTube videos with simple iframe
jonkafton Oct 29, 2024
f5e916f
16:9 aspect ratio for videos
jonkafton Oct 29, 2024
d66ab6e
Revert "Suspense boundaries needed around carousels (useSearchParams(…
jonkafton Oct 29, 2024
f97d90f
Revert "Upgrade @mui/material-nextjs for Next.js v15"
jonkafton Oct 29, 2024
eb5b317
Revert "Upgrade to Next.js v15. Page params are now async"
jonkafton Oct 29, 2024
00ef518
Update test for unit logo
jonkafton Oct 29, 2024
8bb8dcb
Add Nextjs dependency in ol-utilities
jonkafton Oct 29, 2024
cb37c04
Merge branch 'main' into jk/5857-image-optimizations
jonkafton Oct 30, 2024
d75212a
Remove comments
jonkafton Oct 30, 2024
0ce9cf9
Remove unnecessary truthy check
jonkafton Oct 30, 2024
e38f0d9
Separate Unit/Platform Logo
jonkafton Oct 30, 2024
1086dde
Move to peer dependency
jonkafton Oct 30, 2024
36a3982
Bckground src set for topic banner
jonkafton Oct 30, 2024
c868b00
Background src set on other banner backgrounds
jonkafton Oct 31, 2024
9386f27
Remove redundant display breakpoints
jonkafton Oct 31, 2024
6ed1589
Merge branch 'main' into jk/5857-image-optimizations
jonkafton Oct 31, 2024
9fc386a
Apply changes to LearningResourceExpandedV2
jonkafton Oct 31, 2024
97ad4b8
Update test
jonkafton Oct 31, 2024
186b748
Merge branch 'main' into jk/5857-image-optimizations
jonkafton Nov 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion frontends/main/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
"api": "workspace:*",
"formik": "^2.4.6",
"lodash": "^4.17.21",
"next": "^14.2.15",
"next": "^15.0.1",
"ol-ckeditor": "0.0.0",
"ol-components": "0.0.0",
"ol-utilities": "0.0.0",
Expand Down
19 changes: 14 additions & 5 deletions frontends/main/src/app-pages/AboutPage/AboutPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
import * as urls from "@/common/urls"
import React from "react"
import domeImage from "@/public/mit-dome-2.jpg"
import Image from "next/image"

const WHAT_IS_MIT_OPEN_FRAGMENT_IDENTIFIER = "what-is-mit-learn"
const NON_DEGREE_LEARNING_FRAGMENT_IDENTIFIER = "non-degree-learning"
Expand Down Expand Up @@ -81,13 +82,15 @@ const SubHeaderTextContainer = styled.div({
alignSelf: "flex-start",
})

const SubHeaderImage = styled.img({
const SubHeaderImageContainer = styled.div({
flexGrow: 1,
alignSelf: "stretch",
position: "relative",
})

const SubHeaderImage = styled(Image)({
borderRadius: "8px",
backgroundSize: "cover",
backgroundPosition: "center",
backgroundImage: `url(${domeImage.src})`,
objectFit: "cover",
[theme.breakpoints.down("md")]: {
height: "300px",
},
Expand Down Expand Up @@ -165,7 +168,13 @@ const AboutPage: React.FC = () => {
<li>Continue your education at your own pace</li>
</List>
</SubHeaderTextContainer>
<SubHeaderImage />
<SubHeaderImageContainer>
<SubHeaderImage
src={domeImage}
alt="A view of the entablature of MIT's Great Dome"
Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

today i learn...

fill
/>
</SubHeaderImageContainer>
</SubHeaderContainer>
<BodySection>
<HighlightContainer>
Expand Down
6 changes: 5 additions & 1 deletion frontends/main/src/app-pages/ChannelPage/DefaultChannelTemplate.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
CHANNEL_TYPE_BREADCRUMB_TARGETS,
ChannelControls,
} from "./ChannelPageTemplate"
import backgroundSteps from "@/public/images/backgrounds/background_steps.jpg"

const ChildrenContainer = styled.div(({ theme }) => ({
paddingTop: "40px",
Expand Down Expand Up @@ -57,6 +58,7 @@ const DefaultChannelTemplate: React.FC<DefaultChannelTemplateProps> = ({
const channel = useChannelDetail(String(channelType), String(name))
const urlParams = new URLSearchParams(channel.data?.search_filter)
const displayConfiguration = channel.data?.configuration

return (
<>
<Banner
Expand Down Expand Up @@ -87,7 +89,9 @@ const DefaultChannelTemplate: React.FC<DefaultChannelTemplateProps> = ({
title={channel.data?.title}
header={displayConfiguration?.heading}
subHeader={displayConfiguration?.sub_heading}
backgroundUrl={displayConfiguration?.banner_background}
backgroundUrl={
displayConfiguration?.banner_background ?? backgroundSteps
}
extraActions={
<ChannelControlsContainer>
<ChannelControls>
Expand Down
5 changes: 3 additions & 2 deletions frontends/main/src/app-pages/ChannelPage/TopicChannelTemplate.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import {
} from "api/hooks/learningResources"
import { propsNotNil } from "ol-utilities"
import invariant from "tiny-invariant"
import backgroundSteps from "@/public/images/backgrounds/background_steps.jpg"

const ChildrenContainer = styled.div(({ theme }) => ({
paddingTop: "40px",
Expand Down Expand Up @@ -222,6 +223,7 @@ const TopicChannelTemplateInternal: React.FC<
) : (
<BreadcrumbsInternal current={channel.title} />
)

return (
<>
<Banner
Expand All @@ -248,8 +250,7 @@ const TopicChannelTemplateInternal: React.FC<
subHeader={displayConfiguration?.sub_heading}
extraHeader={<TopicChips topic={topic} />}
backgroundUrl={
displayConfiguration?.banner_background ??
"/images/backgrounds/background_steps.jpg"
displayConfiguration?.banner_background ?? backgroundSteps.src
}
extraActions={
<ChannelControlsContainer>
Expand Down
38 changes: 30 additions & 8 deletions frontends/main/src/app-pages/ChannelPage/UnitChannelTemplate.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,16 @@ import {
BannerBackground,
Typography,
VisuallyHidden,
PlatformLogo,
} from "ol-components"
import { OfferedByEnum, SourceTypeEnum } from "api"
import { SearchSubscriptionToggle } from "@/page-components/SearchSubscriptionToggle/SearchSubscriptionToggle"
import { ChannelDetails } from "@/page-components/ChannelDetails/ChannelDetails"
import { useChannelDetail } from "api/hooks/channels"
import ChannelMenu from "@/components/ChannelMenu/ChannelMenu"
import ResourceCarousel, {
ResourceCarouselProps,
} from "@/page-components/ResourceCarousel/ResourceCarousel"
import { SourceTypeEnum } from "api"
import { getSearchParamMap } from "@/common/utils"
import { HOME as HOME_URL, UNITS as UNITS_URL } from "../../common/urls"
import { ChannelTypeEnum } from "api/v0"
Expand All @@ -38,16 +39,24 @@ const FeaturedCoursesCarousel = styled(ResourceCarousel)(({ theme }) => ({
},
}))

const UnitLogo = styled.img(({ theme }) => ({
filter: "saturate(0%) invert(100%)",
maxWidth: "100%",
width: "auto",
height: "50px",
const MobileOnly = styled.div(({ theme }) => ({
display: "contents",
[theme.breakpoints.up("md")]: {
display: "none",
},
}))

const DesktopOnly = styled.div(({ theme }) => ({
display: "contents",
[theme.breakpoints.down("md")]: {
height: "40px",
display: "none",
},
}))

const PlatformLogoInverted = styled(PlatformLogo)({
filter: "saturate(0%) invert(100%)",
})

const BannerContent = styled.div(({ theme }) => ({
display: "flex",
flexDirection: "row",
Expand Down Expand Up @@ -126,7 +135,20 @@ const UnitChannelTemplate: React.FC<UnitChannelTemplateProps> = ({
<ChannelHeader>
<VisuallyHidden>{channel.data?.title}</VisuallyHidden>
{channel.data ? (
<UnitLogo alt="" src={displayConfiguration.logo} />
<>
<DesktopOnly>
<PlatformLogoInverted
unitCode={name as OfferedByEnum}
height={50}
/>
</DesktopOnly>
<MobileOnly>
<PlatformLogoInverted
unitCode={name as OfferedByEnum}
height={40}
/>
</MobileOnly>
Copy link
Contributor Author

@jonkafton jonkafton Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

</>
) : null}
</ChannelHeader>
<Stack gap={{ xs: "16px", lg: "32px" }}>
Expand Down
4 changes: 2 additions & 2 deletions frontends/main/src/app-pages/DepartmentListingPage/DepartmentListingPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import {
RiTerminalBoxLine,
} from "@remixicon/react"
import { HOME } from "@/common/urls"

import backgroundSteps from "@/public/images/backgrounds/background_steps.jpg"
import { aggregateProgramCounts, aggregateCourseCounts } from "@/common/utils"
import { useChannelCounts } from "api/hooks/channels"

Expand Down Expand Up @@ -201,7 +201,6 @@ const DepartmentListingPage: React.FC = () => {
return (
<>
<Banner
backgroundUrl="/images/backgrounds/background_steps.jpg"
title="Browse by Academic Department"
header="At MIT, academic departments span a wide range of disciplines, from science and engineering to humanities. Select a department below to explore all of its non-degree learning offerings."
navText={
Expand All @@ -211,6 +210,7 @@ const DepartmentListingPage: React.FC = () => {
current="Departments"
/>
}
backgroundUrl={backgroundSteps.src}
Copy link
Contributor Author

@jonkafton jonkafton Oct 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Not include Banner in ol-components, move it to main/src/components
  2. 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.
  3. 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.

/>
<Container>
<Grid container>
Expand Down
28 changes: 16 additions & 12 deletions frontends/main/src/app-pages/HomePage/HomePage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
"use client"

import React from "react"
import React, { Suspense } from "react"
import { Container, styled, theme } from "ol-components"
import HeroSearch from "@/page-components/HeroSearch/HeroSearch"
import BrowseTopicsSection from "./BrowseTopicsSection"
Expand Down Expand Up @@ -41,26 +41,30 @@ const MediaCarousel = styled(ResourceCarousel)(({ theme }) => ({
const HomePage: React.FC = () => {
return (
<>
<LearningResourceDrawer />
<Container>
<LearningResourceDrawer />
</Container>
<FullWidthBackground>
<Container>
<HeroSearch />
<section>
<HeroSearch />
<section>
<Suspense>
<FeaturedCoursesCarousel
titleComponent="h2"
title="Featured Courses"
config={carousels.FEATURED_RESOURCES_CAROUSEL}
/>
</section>
</Container>
</Suspense>
</section>
</FullWidthBackground>
<PersonalizeSection />
<Container component="section">
<MediaCarousel
titleComponent="h2"
title="Media"
config={carousels.MEDIA_CAROUSEL}
/>
<Suspense>
<MediaCarousel
titleComponent="h2"
title="Media"
config={carousels.MEDIA_CAROUSEL}
/>
</Suspense>
</Container>
<BrowseTopicsSection />
<TestimonialsSection />
Expand Down
7 changes: 5 additions & 2 deletions frontends/main/src/app-pages/HomePage/PersonalizeSection.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
import React from "react"
import { Typography, styled, Container, ButtonLink } from "ol-components"
import { backgroundSrcSetCSS } from "ol-utilities"
import { useUserMe } from "api/hooks/user"
import * as urls from "@/common/urls"
import personalizeImage from "@/public/images/homepage/personalize-image.png"
import personalizeBgImage from "@/public/images/homepage/personalize-bg.png"

const FullWidthBackground = styled.div(({ theme }) => ({
padding: "80px 0",
background: 'url("/images/homepage/personalize-bg.png") center top no-repeat',
background: `${backgroundSrcSetCSS(personalizeBgImage)} center top no-repeat`,
backgroundSize: "cover",
[theme.breakpoints.down("md")]: {
padding: "40px 0",
Expand Down Expand Up @@ -108,7 +111,7 @@ const PersonalizeSection = () => {
return (
<FullWidthBackground>
<PersonalizeContainer>
<ImageContainer src="/images/homepage/personalize-image.png" alt="" />
<ImageContainer src={personalizeImage.src} alt="" />
<PersonalizeContent />
</PersonalizeContainer>
</FullWidthBackground>
Expand Down
5 changes: 0 additions & 5 deletions frontends/main/src/app-pages/LearningPathListingPage/LearningPathListingPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,11 +84,6 @@ const LearningPathListingPage: React.FC = () => {
src="/images/backgrounds/course_search_banner.png"
className="learningpaths-page"
>
{/* TODO <MetaTags title="Learning Paths" social={false} />
<Helmet>
<meta name="robots" content="noindex,noarchive" />
</Helmet>
*/}
<Container maxWidth="md" style={{ paddingBottom: 100 }}>
<GridContainer>
<GridColumn variant="single-full">
Expand Down
0 ...tterPage/[id]/view/ProgramLetter.test.tsx → .../ProgramLetterPage/ProgramLetter.test.tsx
View file
Open in desktop
File renamed without changes.
0 ...etterPage/[id]/view/ProgramLetterPage.tsx → ...s/ProgramLetterPage/ProgramLetterPage.tsx
View file
Open in desktop
File renamed without changes.
2 changes: 1 addition & 1 deletion frontends/main/src/app-pages/TermsPage/TermsPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
"use client"

import React from "react"
// Not urrently linked to. See https://github.com/mitodl/hq/issues/4639
// Not currently linked to. See https://github.com/mitodl/hq/issues/4639
import {
Breadcrumbs,
Container,
Expand Down
3 changes: 2 additions & 1 deletion frontends/main/src/app-pages/TopicsListingPage/TopicsListingPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,13 @@ import {
} from "ol-components"
import Link from "next/link"
import { propsNotNil } from "ol-utilities"

import { useLearningResourceTopics } from "api/hooks/learningResources"
import { LearningResourceTopic } from "api"
import RootTopicIcon from "@/components/RootTopicIcon/RootTopicIcon"
import { HOME } from "@/common/urls"
import { aggregateProgramCounts, aggregateCourseCounts } from "@/common/utils"
import { useChannelCounts } from "api/hooks/channels"
import backgroundSteps from "@/public/images/backgrounds/background_steps.jpg"

type ChannelSummary = {
id: number | string
Expand Down Expand Up @@ -275,6 +275,7 @@ const TopicsListingPage: React.FC = () => {
}
title="Browse by Topic"
header="Select a topic below to explore relevant learning resources across all Academic and Professional units."
backgroundUrl={backgroundSteps.src}
Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason not to use backgroundSrcSetCSS here?

Copy link
Contributor Author

@jonkafton jonkafton Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/>
<Container>
<Grid container>
Expand Down
Loading
Loading