From bef36f8d45341565aa64f3042b816eda3746221b Mon Sep 17 00:00:00 2001 From: Jon Kafton <939376+jonkafton@users.noreply.github.com> Date: Fri, 28 Jun 2024 20:58:14 +0200 Subject: [PATCH 1/9] Top pick carousel display only if results --- .../pages/DashboardPage/Dashboard.test.tsx | 21 ++++++++- .../src/pages/DashboardPage/DashboardPage.tsx | 44 ++++++++++++++----- frontends/mit-open/src/test-utils/index.tsx | 1 + 3 files changed, 52 insertions(+), 14 deletions(-) diff --git a/frontends/mit-open/src/pages/DashboardPage/Dashboard.test.tsx b/frontends/mit-open/src/pages/DashboardPage/Dashboard.test.tsx index 099bd581da..3557158560 100644 --- a/frontends/mit-open/src/pages/DashboardPage/Dashboard.test.tsx +++ b/frontends/mit-open/src/pages/DashboardPage/Dashboard.test.tsx @@ -4,6 +4,7 @@ import { setMockResponse, within, renderWithProviders, + // waitForElementToBeRemoved, } from "../../test-utils" import { factories, urls } from "api/test-utils" import { Permissions } from "@/common/permissions" @@ -36,7 +37,7 @@ describe("DashboardPage", () => { return responseData } - const setupAPIs = () => { + const setupAPIs = ({ topPicksCount }: { topPicksCount?: number } = {}) => { const profile = factories.user.profile({ preference_search_filters: { topic: factories.learningResources @@ -59,7 +60,10 @@ describe("DashboardPage", () => { profile?.preference_search_filters.learning_format?.includes(format), ) - const topPicks = factories.learningResources.courses({ count: 10 }) + const topPicks = factories.learningResources.courses({ + count: topPicksCount ?? 10, + }) + topPicks.results.forEach((course) => { course.topics = topics?.map((topic) => factories.learningResources.topic({ name: topic }), @@ -338,4 +342,17 @@ describe("DashboardPage", () => { expect(courseTitle).toBeInTheDocument() }) }, 10000) + + test("Does not render the Top picks carousel if no results", async () => { + setupAPIs({ topPicksCount: 0 }) + + renderWithProviders() + + await screen.findByText("Popular") + + const topPicksTitle = screen.getByText("Top picks for you") + expect(topPicksTitle).not.toBeInTheDocument() + + // await waitForElementToBeRemoved(() => screen.queryByText("Top picks for you")) + }) }) diff --git a/frontends/mit-open/src/pages/DashboardPage/DashboardPage.tsx b/frontends/mit-open/src/pages/DashboardPage/DashboardPage.tsx index e7fdc00654..2072b06831 100644 --- a/frontends/mit-open/src/pages/DashboardPage/DashboardPage.tsx +++ b/frontends/mit-open/src/pages/DashboardPage/DashboardPage.tsx @@ -24,15 +24,19 @@ import { Link } from "react-router-dom" import { useUserMe } from "api/hooks/user" import { useLocation } from "react-router" import { UserListListingComponent } from "../UserListListingPage/UserListListingPage" -import { UserList } from "api" -import { ListDetailsComponent } from "../ListDetailsPage/ListDetailsPage" -import { ListType } from "api/constants" -import { manageListDialogs } from "@/page-components/ManageListDialogs/ManageListDialogs" -import { ProfileEditForm } from "./ProfileEditForm" +import { + UserList, + LearningResourcesSearchApiLearningResourcesSearchRetrieveRequest, +} from "api" import { useInfiniteUserListItems, useUserListsDetail, + useLearningResourcesSearch, } from "api/hooks/learningResources" +import { ListDetailsComponent } from "../ListDetailsPage/ListDetailsPage" +import { ListType } from "api/constants" +import { manageListDialogs } from "@/page-components/ManageListDialogs/ManageListDialogs" +import { ProfileEditForm } from "./ProfileEditForm" import { useProfileMeQuery } from "api/hooks/profile" import { TopPicksCarouselConfig, @@ -329,6 +333,28 @@ const UserListDetailsTab: React.FC = (props) => { ) } +const TopPicksCarousel: React.FC = () => { + const { isLoading: isLoadingProfile, data: profile } = useProfileMeQuery() + const config = TopPicksCarouselConfig(profile) + const { data, isLoading } = useLearningResourcesSearch( + config[0].data + .params as LearningResourcesSearchApiLearningResourcesSearchRetrieveRequest, + ) + + if (!isLoading && !data?.results?.length) { + return null + } + return ( + + + + ) +} + const DashboardPage: React.FC = () => { const { isLoading: isLoadingUser, data: user } = useUserMe() const { isLoading: isLoadingProfile, data: profile } = useProfileMeQuery() @@ -442,13 +468,7 @@ const DashboardPage: React.FC = () => { - - - + {topics?.map((topic) => ( Date: Fri, 28 Jun 2024 21:05:03 +0200 Subject: [PATCH 2/9] Remove not in document test - unreliable --- .../src/pages/DashboardPage/Dashboard.test.tsx | 18 ++---------------- .../LearningResourceCard.tsx | 2 +- 2 files changed, 3 insertions(+), 17 deletions(-) diff --git a/frontends/mit-open/src/pages/DashboardPage/Dashboard.test.tsx b/frontends/mit-open/src/pages/DashboardPage/Dashboard.test.tsx index 3557158560..80ef3cd57d 100644 --- a/frontends/mit-open/src/pages/DashboardPage/Dashboard.test.tsx +++ b/frontends/mit-open/src/pages/DashboardPage/Dashboard.test.tsx @@ -4,7 +4,6 @@ import { setMockResponse, within, renderWithProviders, - // waitForElementToBeRemoved, } from "../../test-utils" import { factories, urls } from "api/test-utils" import { Permissions } from "@/common/permissions" @@ -37,7 +36,7 @@ describe("DashboardPage", () => { return responseData } - const setupAPIs = ({ topPicksCount }: { topPicksCount?: number } = {}) => { + const setupAPIs = () => { const profile = factories.user.profile({ preference_search_filters: { topic: factories.learningResources @@ -61,7 +60,7 @@ describe("DashboardPage", () => { ) const topPicks = factories.learningResources.courses({ - count: topPicksCount ?? 10, + count: 10, }) topPicks.results.forEach((course) => { @@ -342,17 +341,4 @@ describe("DashboardPage", () => { expect(courseTitle).toBeInTheDocument() }) }, 10000) - - test("Does not render the Top picks carousel if no results", async () => { - setupAPIs({ topPicksCount: 0 }) - - renderWithProviders() - - await screen.findByText("Popular") - - const topPicksTitle = screen.getByText("Top picks for you") - expect(topPicksTitle).not.toBeInTheDocument() - - // await waitForElementToBeRemoved(() => screen.queryByText("Top picks for you")) - }) }) diff --git a/frontends/ol-components/src/components/LearningResourceCard/LearningResourceCard.tsx b/frontends/ol-components/src/components/LearningResourceCard/LearningResourceCard.tsx index b37d9dbf79..a0adc69f5f 100644 --- a/frontends/ol-components/src/components/LearningResourceCard/LearningResourceCard.tsx +++ b/frontends/ol-components/src/components/LearningResourceCard/LearningResourceCard.tsx @@ -97,7 +97,7 @@ const StartDate: React.FC<{ resource: LearningResource; size?: Size }> = ({ resource, size, }) => { - const startDate = getStartDate(resource) + const startDate = getStartDate(resource, size) if (!startDate) return null From 3b328b920c78990b8b9a1b83106ccdb05d313f2a Mon Sep 17 00:00:00 2001 From: Jon Kafton <939376+jonkafton@users.noreply.github.com> Date: Fri, 28 Jun 2024 21:34:04 +0200 Subject: [PATCH 3/9] Not used anywhere --- frontends/mit-open/src/pages/DashboardPage/Dashboard.test.tsx | 4 +--- frontends/mit-open/src/test-utils/index.tsx | 1 - 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/frontends/mit-open/src/pages/DashboardPage/Dashboard.test.tsx b/frontends/mit-open/src/pages/DashboardPage/Dashboard.test.tsx index 80ef3cd57d..584a835317 100644 --- a/frontends/mit-open/src/pages/DashboardPage/Dashboard.test.tsx +++ b/frontends/mit-open/src/pages/DashboardPage/Dashboard.test.tsx @@ -59,9 +59,7 @@ describe("DashboardPage", () => { profile?.preference_search_filters.learning_format?.includes(format), ) - const topPicks = factories.learningResources.courses({ - count: 10, - }) + const topPicks = factories.learningResources.courses({ count: 10 }) topPicks.results.forEach((course) => { course.topics = topics?.map((topic) => diff --git a/frontends/mit-open/src/test-utils/index.tsx b/frontends/mit-open/src/test-utils/index.tsx index db6bfe6c92..29d9de80f9 100644 --- a/frontends/mit-open/src/test-utils/index.tsx +++ b/frontends/mit-open/src/test-utils/index.tsx @@ -156,7 +156,6 @@ export { fireEvent, waitFor, renderHook, - waitForElementToBeRemoved, } from "@testing-library/react" export { default as user } from "@testing-library/user-event" From 18a77a86c44e15fa8e2b591e166dfec4000c09d5 Mon Sep 17 00:00:00 2001 From: Jon Kafton <939376+jonkafton@users.noreply.github.com> Date: Wed, 3 Jul 2024 18:20:19 +0200 Subject: [PATCH 4/9] Filter carousels without results --- .../api/src/hooks/learningResources/index.ts | 7 + .../ResourceCarousel/ResourceCarousel.tsx | 195 +++++++----------- .../src/pages/DashboardPage/DashboardPage.tsx | 102 ++++----- 3 files changed, 117 insertions(+), 187 deletions(-) diff --git a/frontends/api/src/hooks/learningResources/index.ts b/frontends/api/src/hooks/learningResources/index.ts index a97bc4509c..13b930754a 100644 --- a/frontends/api/src/hooks/learningResources/index.ts +++ b/frontends/api/src/hooks/learningResources/index.ts @@ -427,6 +427,13 @@ const useSchoolsList = () => { return useQuery(learningResources.schools()) } +/* + * Not intended to be imported except for special cases. + * It's used in the ResourceCarousel to dynamically build a single useQueries hook + * from config because a React component cannot conditionally call hooks during renders. + */ +export { default as learningResourcesKeyFactory } from "./keyFactory" + export { useLearningResourcesList, useFeaturedLearningResourcesList, diff --git a/frontends/mit-open/src/page-components/ResourceCarousel/ResourceCarousel.tsx b/frontends/mit-open/src/page-components/ResourceCarousel/ResourceCarousel.tsx index b88abefe26..6e3a3ca6f0 100644 --- a/frontends/mit-open/src/page-components/ResourceCarousel/ResourceCarousel.tsx +++ b/frontends/mit-open/src/page-components/ResourceCarousel/ResourceCarousel.tsx @@ -1,9 +1,9 @@ import React from "react" import { useFeaturedLearningResourcesList, - useLearningResourcesList, - useLearningResourcesSearch, + learningResourcesKeyFactory, } from "api/hooks/learningResources" + import { Carousel, TabButton, @@ -13,14 +13,10 @@ import { styled, Typography, } from "ol-components" -import type { - TabConfig, - ResourceDataSource, - SearchDataSource, - FeaturedDataSource, -} from "./types" -import { LearningResource } from "api" +import type { TabConfig, FeaturedDataSource } from "./types" +import { LearningResource, PaginatedLearningResourceList } from "api" import { ResourceCard } from "../ResourceCard/ResourceCard" +import { useQueries, UseQueryResult } from "@tanstack/react-query" const StyledCarousel = styled(Carousel)({ /** @@ -40,18 +36,6 @@ const StyledCarousel = styled(Carousel)({ }, }) -type DataPanelProps = { - dataConfig: T - isLoading?: boolean - children: ({ - resources, - childrenLoading, - }: { - resources: LearningResource[] - childrenLoading: boolean - }) => React.ReactNode -} - type LoadTabButtonProps = { config: FeaturedDataSource label: React.ReactNode @@ -59,73 +43,6 @@ type LoadTabButtonProps = { value: string } -const ResourcesData: React.FC> = ({ - dataConfig, - children, -}) => { - const { data, isLoading } = useLearningResourcesList(dataConfig.params) - return children({ - resources: data?.results ?? [], - childrenLoading: isLoading, - }) -} - -const SearchData: React.FC> = ({ - dataConfig, - children, -}) => { - const { data, isLoading } = useLearningResourcesSearch(dataConfig.params) - return children({ - resources: data?.results ?? [], - childrenLoading: isLoading, - }) -} - -const FeaturedData: React.FC> = ({ - dataConfig, - children, -}) => { - const { data, isLoading } = useFeaturedLearningResourcesList( - dataConfig.params, - ) - return children({ - resources: data?.results ?? [], - childrenLoading: isLoading, - }) -} - -/** - * A wrapper to load data based `TabConfig.data`. - * - * For each `TabConfig.data.type`, a different API endpoint, and hence - * react-query hook, is used. Since hooks can't be called conditionally within - * a single component, each type of data is handled in a separate component. - */ -const DataPanel: React.FC = ({ - dataConfig, - isLoading, - children, -}) => { - if (!isLoading) { - switch (dataConfig.type) { - case "resources": - return {children} - case "lr_search": - return {children} - case "lr_featured": - return {children} - default: - // @ts-expect-error This will always be an error if the switch statement - // is exhaustive since dataConfig will have type `never` - throw new Error(`Unknown data type: ${dataConfig.type}`) - } - } else - return children({ - resources: [], - childrenLoading: true, - }) -} - /** * Tab button that loads the resource, so we can determine if it needs to be * displayed or not. This shouldn't cause double-loading since React Query @@ -195,48 +112,44 @@ const TabsList = styled(TabButtonList)({ type ContentProps = { resources: LearningResource[] - isLoading?: boolean + childrenLoading?: boolean tabConfig: TabConfig } type PanelChildrenProps = { config: TabConfig[] + queries: UseQueryResult[] children: (props: ContentProps) => React.ReactNode - isLoading?: boolean } const PanelChildren: React.FC = ({ config, + queries, children, - isLoading, }) => { if (config.length === 1) { - return ( - - {({ resources, childrenLoading }) => - children({ - resources, - isLoading: childrenLoading || isLoading, - tabConfig: config[0], - }) - } - - ) + const { data, isLoading } = queries[0] + const resources = data?.results ?? [] + return children({ + resources, + childrenLoading: isLoading, + tabConfig: config[0], + }) } return ( <> - {config.map((tabConfig, index) => ( - - - {({ resources, childrenLoading }) => - children({ - resources, - isLoading: childrenLoading || isLoading, - tabConfig, - }) - } - - - ))} + {config.map((tabConfig, index) => { + const { data, isLoading } = queries[index] + const resources = data?.results ?? [] + return ( + + {children({ + resources, + childrenLoading: isLoading, + tabConfig, + })} + + ) + })} ) } @@ -258,6 +171,7 @@ type ResourceCarouselProps = { title: string className?: string isLoading?: boolean + "data-testid"?: string } /** * A tabbed carousel that fetches resources based on the configuration provided. @@ -275,12 +189,37 @@ const ResourceCarousel: React.FC = ({ title, className, isLoading, + "data-testid": dataTestId, }) => { const [tab, setTab] = React.useState("0") const [ref, setRef] = React.useState(null) + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const queries = useQueries({ + // const queries = useQueries[]>({ + queries: config.map((tab) => { + switch (tab.data.type) { + case "resources": + return learningResourcesKeyFactory.list(tab.data.params) + case "lr_search": + return learningResourcesKeyFactory.search(tab.data.params) + case "lr_featured": + return learningResourcesKeyFactory.featured(tab.data.params) + } + }), + }) + + if ( + !isLoading && + !queries.find( + (query) => !query.isLoading && (query.data as { count: number })?.count, + ) + ) { + return null + } + return ( - + {title} @@ -288,8 +227,15 @@ const ResourceCarousel: React.FC = ({ {config.length > 1 ? ( setTab(newValue)}> - {config.map((tabConfig, index) => - tabConfig.data.type === "lr_featured" ? ( + {config.map((tabConfig, index) => { + if ( + !isLoading && + !queries[index].isLoading && + !(queries[index].data as { count: number })?.count + ) { + return null + } + return tabConfig.data.type === "lr_featured" ? ( = ({ label={tabConfig.label} value={index.toString()} /> - ), - )} + ) + })} ) : null} - - {({ resources, isLoading: childrenLoading, tabConfig }) => ( + [] + } + > + {({ resources, childrenLoading, tabConfig }) => ( {isLoading || childrenLoading ? Array.from({ length: 6 }).map((_, index) => ( diff --git a/frontends/mit-open/src/pages/DashboardPage/DashboardPage.tsx b/frontends/mit-open/src/pages/DashboardPage/DashboardPage.tsx index d902c750af..a8917dc3bb 100644 --- a/frontends/mit-open/src/pages/DashboardPage/DashboardPage.tsx +++ b/frontends/mit-open/src/pages/DashboardPage/DashboardPage.tsx @@ -24,14 +24,10 @@ import { Link } from "react-router-dom" import { useUserMe } from "api/hooks/user" import { useLocation } from "react-router" import { UserListListingComponent } from "../UserListListingPage/UserListListingPage" -import { - UserList, - LearningResourcesSearchApiLearningResourcesSearchRetrieveRequest, -} from "api" +import { UserList } from "api" import { useInfiniteUserListItems, useUserListsDetail, - useLearningResourcesSearch, } from "api/hooks/learningResources" import { ListDetailsComponent } from "../ListDetailsPage/ListDetailsPage" import { ListType } from "api/constants" @@ -252,7 +248,7 @@ const HomeHeaderRight = styled.div(({ theme }) => ({ }, })) -const CarouselContainer = styled.div(({ theme }) => ({ +const StyledResourceCarousel = styled(ResourceCarousel)(({ theme }) => ({ padding: "40px 0", [theme.breakpoints.down("md")]: { padding: "16px 0", @@ -333,28 +329,6 @@ const UserListDetailsTab: React.FC = (props) => { ) } -const TopPicksCarousel: React.FC = () => { - const { isLoading: isLoadingProfile, data: profile } = useProfileMeQuery() - const config = TopPicksCarouselConfig(profile) - const { data, isLoading } = useLearningResourcesSearch( - config[0].data - .params as LearningResourcesSearchApiLearningResourcesSearchRetrieveRequest, - ) - - if (!isLoading && !data?.results?.length) { - return null - } - return ( - - - - ) -} - const DashboardPage: React.FC = () => { const { isLoading: isLoadingUser, data: user } = useUserMe() const { isLoading: isLoadingProfile, data: profile } = useProfileMeQuery() @@ -468,48 +442,46 @@ const DashboardPage: React.FC = () => { - - {topics?.map((topic) => ( - + {topics?.map((topic, index) => ( + - - + /> ))} {certification === true ? ( - - - - ) : ( - - - - )} - - - - - - + )} + + {userListAction === "list" ? ( From 13c74e56696871239ca65ba8d2435cc3d042f751 Mon Sep 17 00:00:00 2001 From: Jon Kafton <939376+jonkafton@users.noreply.github.com> Date: Mon, 8 Jul 2024 17:26:32 +0200 Subject: [PATCH 5/9] Update tests to reflect load states --- .../ResourceCarousel/ResourceCarousel.test.tsx | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/frontends/mit-open/src/page-components/ResourceCarousel/ResourceCarousel.test.tsx b/frontends/mit-open/src/page-components/ResourceCarousel/ResourceCarousel.test.tsx index 358f21718d..ed54fb90f0 100644 --- a/frontends/mit-open/src/page-components/ResourceCarousel/ResourceCarousel.test.tsx +++ b/frontends/mit-open/src/page-components/ResourceCarousel/ResourceCarousel.test.tsx @@ -83,19 +83,13 @@ describe("ResourceCarousel", () => { }, ] - const { resources, resolve } = setupApis({ autoResolve: false }) + const { resources } = setupApis({ autoResolve: true }) renderWithProviders( , ) - expectLastProps(spyLearningResourceCard, { - isLoading: true, - ...cardProps, - }) - resolve() - - const tabs = screen.getAllByRole("tab") + const tabs = await screen.findAllByRole("tab") expect(tabs).toHaveLength(2) expect(tabs[0]).toHaveTextContent("Resources") @@ -178,7 +172,7 @@ describe("ResourceCarousel", () => { expect(urlParams.get("professional")).toEqual("true") }) - it("Shows the correct title", () => { + it("Shows the correct title", async () => { const config: ResourceCarouselProps["config"] = [ { label: "Resources", @@ -193,8 +187,7 @@ describe("ResourceCarousel", () => { renderWithProviders( , ) - expect( - screen.getByRole("heading", { name: "My Favorite Carousel" }), - ).toBeInTheDocument() + + await screen.findByRole("heading", { name: "My Favorite Carousel" }) }) }) From 4af20603c706fc7071d331f4b45444cdb4ff206d Mon Sep 17 00:00:00 2001 From: Jon Kafton <939376+jonkafton@users.noreply.github.com> Date: Mon, 8 Jul 2024 17:53:06 +0200 Subject: [PATCH 6/9] More specific 'any' and reasoning --- .../ResourceCarousel/ResourceCarousel.tsx | 48 ++++++++++++------- 1 file changed, 30 insertions(+), 18 deletions(-) diff --git a/frontends/mit-open/src/page-components/ResourceCarousel/ResourceCarousel.tsx b/frontends/mit-open/src/page-components/ResourceCarousel/ResourceCarousel.tsx index 6e3a3ca6f0..44b6b1c065 100644 --- a/frontends/mit-open/src/page-components/ResourceCarousel/ResourceCarousel.tsx +++ b/frontends/mit-open/src/page-components/ResourceCarousel/ResourceCarousel.tsx @@ -16,7 +16,11 @@ import { import type { TabConfig, FeaturedDataSource } from "./types" import { LearningResource, PaginatedLearningResourceList } from "api" import { ResourceCard } from "../ResourceCard/ResourceCard" -import { useQueries, UseQueryResult } from "@tanstack/react-query" +import { + useQueries, + UseQueryResult, + UseQueryOptions, +} from "@tanstack/react-query" const StyledCarousel = styled(Carousel)({ /** @@ -194,21 +198,31 @@ const ResourceCarousel: React.FC = ({ const [tab, setTab] = React.useState("0") const [ref, setRef] = React.useState(null) - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const queries = useQueries({ - // const queries = useQueries[]>({ - queries: config.map((tab) => { - switch (tab.data.type) { - case "resources": - return learningResourcesKeyFactory.list(tab.data.params) - case "lr_search": - return learningResourcesKeyFactory.search(tab.data.params) - case "lr_featured": - return learningResourcesKeyFactory.featured(tab.data.params) - } - }), + const queries = useQueries({ + queries: config.map( + ( + tab, + ): UseQueryOptions< + PaginatedLearningResourceList, + unknown, + unknown, + // The factory-generated types for queryKeys are very specific (tuples not arrays) + // and assignable to the loose QueryKey (readonly unknown[]) on the UseQueryOptions generic. + // But! as a queryFn arg the more specific QueryKey cannot be assigned to the looser QueryKey. + // eslint-disable-next-line @typescript-eslint/no-explicit-any + any + > => { + switch (tab.data.type) { + case "resources": + return learningResourcesKeyFactory.list(tab.data.params) + case "lr_search": + return learningResourcesKeyFactory.search(tab.data.params) + case "lr_featured": + return learningResourcesKeyFactory.featured(tab.data.params) + } + }, + ), }) - if ( !isLoading && !queries.find( @@ -257,9 +271,7 @@ const ResourceCarousel: React.FC = ({ [] - } + queries={queries as UseQueryResult[]} > {({ resources, childrenLoading, tabConfig }) => ( From 50ff808174e1b93875c009aa0a4a16ec92431d72 Mon Sep 17 00:00:00 2001 From: Jon Kafton <939376+jonkafton@users.noreply.github.com> Date: Mon, 8 Jul 2024 21:20:34 +0200 Subject: [PATCH 7/9] Set count in tests --- .../src/pages/DashboardPage/Dashboard.test.tsx | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/frontends/mit-open/src/pages/DashboardPage/Dashboard.test.tsx b/frontends/mit-open/src/pages/DashboardPage/Dashboard.test.tsx index 584a835317..0e6b9ff33e 100644 --- a/frontends/mit-open/src/pages/DashboardPage/Dashboard.test.tsx +++ b/frontends/mit-open/src/pages/DashboardPage/Dashboard.test.tsx @@ -26,7 +26,7 @@ describe("DashboardPage", () => { suggestions: [], aggregations: {}, }, - count: 0, + count: results.length, results: results, next: null, previous: null, @@ -72,6 +72,7 @@ describe("DashboardPage", () => { : { code: "online", name: "Online" }, ) }) + const topicsCourses: CourseResource[] = [] topics?.forEach((topic) => { const topicCourses = factories.learningResources.courses({ count: 10 }) @@ -268,13 +269,12 @@ describe("DashboardPage", () => { const topPicksTitle = await screen.findByText("Top picks for you") expect(topPicksTitle).toBeInTheDocument() - const topPicksCarousel = await screen.findByTestId("top-picks-carousel") - topPicks.results.forEach(async (course) => { - const courseTitle = await within(topPicksCarousel).findByText( - course.title, - ) - expect(courseTitle).toBeInTheDocument() - }) + + await Promise.all( + topPicks.results.map((course) => { + return screen.findAllByText(course.title) + }), + ) profile.preference_search_filters.topic?.forEach(async (topic) => { const topicTitle = await screen.findByText(`Popular courses in ${topic}`) From 18eeb397fc6ddb7b424d647e9c5a0732cf860fac Mon Sep 17 00:00:00 2001 From: Jon Kafton <939376+jonkafton@users.noreply.github.com> Date: Mon, 8 Jul 2024 21:58:50 +0200 Subject: [PATCH 8/9] Wait for all tabs to have loaded before removing if empty (fix layout flash) --- .../ResourceCarousel/ResourceCarousel.tsx | 12 ++++++------ .../src/pages/ChannelPage/ChannelPage.test.tsx | 17 +++++++++-------- 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/frontends/mit-open/src/page-components/ResourceCarousel/ResourceCarousel.tsx b/frontends/mit-open/src/page-components/ResourceCarousel/ResourceCarousel.tsx index 44b6b1c065..5f9cbf6f33 100644 --- a/frontends/mit-open/src/page-components/ResourceCarousel/ResourceCarousel.tsx +++ b/frontends/mit-open/src/page-components/ResourceCarousel/ResourceCarousel.tsx @@ -133,6 +133,7 @@ const PanelChildren: React.FC = ({ if (config.length === 1) { const { data, isLoading } = queries[0] const resources = data?.results ?? [] + return children({ resources, childrenLoading: isLoading, @@ -144,6 +145,7 @@ const PanelChildren: React.FC = ({ {config.map((tabConfig, index) => { const { data, isLoading } = queries[index] const resources = data?.results ?? [] + return ( {children({ @@ -223,12 +225,10 @@ const ResourceCarousel: React.FC = ({ }, ), }) - if ( - !isLoading && - !queries.find( - (query) => !query.isLoading && (query.data as { count: number })?.count, - ) - ) { + + const allChildrenLoaded = queries.every(({ isLoading }) => !isLoading) + const allChildrenEmpty = queries.every(({ data }) => !data?.count) + if (!isLoading && allChildrenLoaded && allChildrenEmpty) { return null } diff --git a/frontends/mit-open/src/pages/ChannelPage/ChannelPage.test.tsx b/frontends/mit-open/src/pages/ChannelPage/ChannelPage.test.tsx index 0797f67eb4..55c0606839 100644 --- a/frontends/mit-open/src/pages/ChannelPage/ChannelPage.test.tsx +++ b/frontends/mit-open/src/pages/ChannelPage/ChannelPage.test.tsx @@ -35,7 +35,7 @@ const setupApis = ( ) setMockResponse.get( expect.stringContaining(urls.learningResources.featured()), - factories.learningResources.resources({ count: 0 }), + factories.learningResources.resources({ count: 10 }), ) const urlParams = new URLSearchParams(channelPatch?.search_filter) @@ -132,13 +132,14 @@ describe("ChannelPage", () => { undefined, ) }) - expect( - makeRequest.mock.calls.filter(([method, url]) => { - return ( - method === "get" && url.includes(urls.learningResources.featured()) - ) - }).length, - ).toBe(1) + + await waitFor(() => { + expect(makeRequest).toHaveBeenCalledWith( + "get", + urls.learningResources.featured({ limit: 12 }), + undefined, + ) + }) }) it("Does not display a featured carousel if the channel type is not 'unit'", async () => { const { channel } = setupApis({ From c9ef60cfd5850f6bfe7b8a58355ccdfb087bd19c Mon Sep 17 00:00:00 2001 From: Jon Kafton <939376+jonkafton@users.noreply.github.com> Date: Tue, 9 Jul 2024 19:12:38 +0200 Subject: [PATCH 9/9] Remove loading logic on lr_featured tabs --- .../ResourceCarousel/ResourceCarousel.tsx | 41 ++----------------- .../mit-open/src/pages/HomePage/HomePage.tsx | 1 + 2 files changed, 5 insertions(+), 37 deletions(-) diff --git a/frontends/mit-open/src/page-components/ResourceCarousel/ResourceCarousel.tsx b/frontends/mit-open/src/page-components/ResourceCarousel/ResourceCarousel.tsx index 5f9cbf6f33..feac14bc43 100644 --- a/frontends/mit-open/src/page-components/ResourceCarousel/ResourceCarousel.tsx +++ b/frontends/mit-open/src/page-components/ResourceCarousel/ResourceCarousel.tsx @@ -1,9 +1,5 @@ import React from "react" -import { - useFeaturedLearningResourcesList, - learningResourcesKeyFactory, -} from "api/hooks/learningResources" - +import { learningResourcesKeyFactory } from "api/hooks/learningResources" import { Carousel, TabButton, @@ -13,7 +9,7 @@ import { styled, Typography, } from "ol-components" -import type { TabConfig, FeaturedDataSource } from "./types" +import type { TabConfig } from "./types" import { LearningResource, PaginatedLearningResourceList } from "api" import { ResourceCard } from "../ResourceCard/ResourceCard" import { @@ -40,28 +36,6 @@ const StyledCarousel = styled(Carousel)({ }, }) -type LoadTabButtonProps = { - config: FeaturedDataSource - label: React.ReactNode - key: number - value: string -} - -/** - * Tab button that loads the resource, so we can determine if it needs to be - * displayed or not. This shouldn't cause double-loading since React Query - * should only run the thing once - when you switch into the tab, the data - * should already be in the cache. - */ - -const LoadFeaturedTabButton: React.FC = (props) => { - const { data, isLoading } = useFeaturedLearningResourcesList( - props.config.params, - ) - - return !isLoading && data && data.count > 0 ? : null -} - const HeaderRow = styled.div(({ theme }) => ({ display: "flex", flexWrap: "wrap", @@ -245,18 +219,11 @@ const ResourceCarousel: React.FC = ({ if ( !isLoading && !queries[index].isLoading && - !(queries[index].data as { count: number })?.count + !queries[index].data?.count ) { return null } - return tabConfig.data.type === "lr_featured" ? ( - - ) : ( + return ( ({ marginTop: "0px", }, })) + const MediaCarousel = styled(ResourceCarousel)(({ theme }) => ({ margin: "80px 0", minHeight: "388px",