From 2a6eca5df81283175cd656bc5a6e14aacf799fe0 Mon Sep 17 00:00:00 2001 From: Anastasia Beglova Date: Tue, 4 Aug 2026 15:06:53 -0400 Subject: [PATCH 1/8] mitxonline course numbers (#3703) --- learning_resources/etl/loaders.py | 2 +- learning_resources/etl/loaders_test.py | 45 ++++++++++++++++++++ learning_resources/etl/mitxonline.py | 29 +++++++++++-- learning_resources/etl/mitxonline_test.py | 52 ++++++++++++----------- 4 files changed, 100 insertions(+), 28 deletions(-) diff --git a/learning_resources/etl/loaders.py b/learning_resources/etl/loaders.py index 62389d29ba..0460777b29 100644 --- a/learning_resources/etl/loaders.py +++ b/learning_resources/etl/loaders.py @@ -576,7 +576,7 @@ def load_course( if config.fetch_only or not learning_resource: return learning_resource - Course.objects.get_or_create( + Course.objects.update_or_create( learning_resource=learning_resource, defaults=course_data ) diff --git a/learning_resources/etl/loaders_test.py b/learning_resources/etl/loaders_test.py index 9927387278..fa8c962b4a 100644 --- a/learning_resources/etl/loaders_test.py +++ b/learning_resources/etl/loaders_test.py @@ -28,6 +28,7 @@ from learning_resources.etl import loaders from learning_resources.etl.constants import ( CourseLoaderConfig, + CourseNumberType, ETLSource, ProgramLoaderConfig, ) @@ -670,6 +671,50 @@ def test_load_course( # noqa: PLR0913, PLR0912, PLR0915 assert getattr(result, key) == value, f"Property {key} should equal {value}" +def test_load_course_updates_course_numbers(mock_upsert_tasks): + """load_course should replace course_numbers on an existing course""" + platform = LearningResourcePlatformFactory.create() + course = CourseFactory.create( + learning_resource__runs=[], + platform=platform.code, + course_numbers=[ + { + "value": "old-number", + "department": None, + "listing_type": CourseNumberType.primary.name, + "primary": True, + "sort_coursenum": "old-number", + } + ], + ) + learning_resource = course.learning_resource + + new_course_numbers = [ + { + "value": "18.03.1x", + "department": None, + "listing_type": CourseNumberType.primary.name, + "primary": True, + "sort_coursenum": "18.03.1x", + } + ] + props = { + "readable_id": learning_resource.readable_id, + "platform": platform.code, + "title": learning_resource.title, + "url": learning_resource.url, + "published": learning_resource.published, + "runs": [], + "course": {"course_numbers": new_course_numbers}, + } + + load_course(props, [], [], config=CourseLoaderConfig(prune=True)) + + assert Course.objects.count() == 1 + course.refresh_from_db() + assert course.course_numbers == new_course_numbers + + def test_load_course_bad_platform(mocker): """A bad platform should log an exception and not create the course""" mock_log = mocker.patch("learning_resources.etl.loaders.log.exception") diff --git a/learning_resources/etl/mitxonline.py b/learning_resources/etl/mitxonline.py index 6c53d4b816..4b5518a44e 100644 --- a/learning_resources/etl/mitxonline.py +++ b/learning_resources/etl/mitxonline.py @@ -4,7 +4,7 @@ import logging import re from collections.abc import Iterator -from datetime import UTC +from datetime import UTC, datetime from decimal import Decimal from urllib.parse import parse_qs, urljoin, urlparse @@ -388,6 +388,25 @@ def _transform_course(course): ] has_certification = parse_certification(OFFERED_BY["code"], runs) strip_enrollment_modes(runs) + # Course runs each carry a course_number. Order the runs by start date + # (latest first) so the latest run supplies the primary course number, then + # collect the distinct values with the remaining numbers as cross-listed. + # This replaces the course readable_id as the source of course numbers. + runs_by_recency = sorted( + (course_run for course_run in course["courseruns"] if course_run), + key=lambda course_run: ( + _parse_datetime( + course_run.get("start_date") or course_run.get("enrollment_start") + ) + or datetime.min.replace(tzinfo=UTC) + ), + reverse=True, + ) + course_numbers = [] + for course_run in runs_by_recency: + course_number = course_run.get("course_number") + if course_number and course_number not in course_numbers: + course_numbers.append(course_number) return { "readable_id": course["readable_id"], "platform": PlatformType.mitxonline.name, @@ -401,8 +420,12 @@ def _transform_course(course): "force_ingest": course.get("ingest_content_files_for_ai", False), "course": { "course_numbers": generate_course_numbers_json( - course["readable_id"], is_ocw=False - ), + course_numbers[0], + extra_nums=course_numbers[1:], + is_ocw=False, + ) + if course_numbers + else [], }, "published": bool( parse_page_attribute(course, "page_url") diff --git a/learning_resources/etl/mitxonline_test.py b/learning_resources/etl/mitxonline_test.py index 4653855059..8b9373ffd0 100644 --- a/learning_resources/etl/mitxonline_test.py +++ b/learning_resources/etl/mitxonline_test.py @@ -4,7 +4,6 @@ # pylint: disable=redefined-outer-name from datetime import UTC, datetime -from unittest.mock import ANY from urllib.parse import parse_qs, urlparse import pytest @@ -18,7 +17,7 @@ PlatformType, RunStatus, ) -from learning_resources.etl.constants import CourseNumberType, ETLSource +from learning_resources.etl.constants import ETLSource from learning_resources.etl.mitxonline import ( OFFERED_BY, _fetch_courses_by_ids, @@ -42,6 +41,7 @@ transform_topics, ) from learning_resources.etl.utils import ( + generate_course_numbers_json, get_department_id_by_name, parse_certification, parse_string_to_int, @@ -54,6 +54,30 @@ pytestmark = pytest.mark.django_db +def _expected_course_numbers(course_data): + """Build expected course_numbers json from a course's runs (latest run first).""" + runs_by_recency = sorted( + (course_run for course_run in course_data["courseruns"] if course_run), + key=lambda course_run: ( + _parse_datetime( + course_run.get("start_date") or course_run.get("enrollment_start") + ) + or datetime.min.replace(tzinfo=UTC) + ), + reverse=True, + ) + course_numbers = [] + for course_run in runs_by_recency: + course_number = course_run.get("course_number") + if course_number and course_number not in course_numbers: + course_numbers.append(course_number) + if not course_numbers: + return [] + return generate_course_numbers_json( + course_numbers[0], extra_nums=course_numbers[1:], is_ocw=False + ) + + @pytest.fixture def mock_mitxonline_programs_data(): """Mock mitxonline data""" @@ -718,17 +742,7 @@ def test_mitxonline_transform_programs( course_data["topics"], OFFERED_BY["code"] ), "runs": runs, - "course": { - "course_numbers": [ - { - "value": course_data["readable_id"], - "department": ANY, - "listing_type": CourseNumberType.primary.value, - "primary": True, - "sort_coursenum": course_data["readable_id"], - } - ] - }, + "course": {"course_numbers": _expected_course_numbers(course_data)}, "position": len(expected_courses), } ) @@ -896,17 +910,7 @@ def test_mitxonline_transform_courses(mock_mitxonline_courses_data, mocker, sett else None ), "runs": runs, - "course": { - "course_numbers": [ - { - "value": course_data["readable_id"], - "department": ANY, - "listing_type": CourseNumberType.primary.value, - "primary": True, - "sort_coursenum": course_data["readable_id"], - } - ] - }, + "course": {"course_numbers": _expected_course_numbers(course_data)}, "availability": course_data["availability"], "format": [Format.asynchronous.name], "pace": [Pace.instructor_paced.name], From fc4ae34db0a042a4b16c1ee0521d4cf64a0e9a8b Mon Sep 17 00:00:00 2001 From: Anastasia Beglova Date: Tue, 4 Aug 2026 15:08:50 -0400 Subject: [PATCH 2/8] Include facets from urls in channel pages (#3695) --- .../ChannelPage/ChannelSearch.test.tsx | 56 +++++++++++++++++++ .../app-pages/ChannelPage/ChannelSearch.tsx | 16 +++++- .../app-pages/ChannelPage/searchRequests.ts | 24 +++++++- .../(site)/c/[channelType]/[name]/page.tsx | 9 +++ 4 files changed, 101 insertions(+), 4 deletions(-) diff --git a/frontends/main/src/app-pages/ChannelPage/ChannelSearch.test.tsx b/frontends/main/src/app-pages/ChannelPage/ChannelSearch.test.tsx index 57dbaad185..796a61d6fc 100644 --- a/frontends/main/src/app-pages/ChannelPage/ChannelSearch.test.tsx +++ b/frontends/main/src/app-pages/ChannelPage/ChannelSearch.test.tsx @@ -332,6 +332,62 @@ describe("ChannelSearch", () => { }, ) + test("Shows and aggregates a facet that is in the URL but not shown by default", async () => { + const { channel } = setMockApiResponses({ + channelPatch: { channel_type: ChannelTypeEnum.Unit }, + search: { + count: 700, + metadata: { + aggregations: { + level: [{ key: "graduate", doc_count: 100 }], + }, + suggestions: [], + }, + }, + }) + + renderWithProviders(, { + url: `/c/${channel.channel_type}/${channel.name}/?level=graduate`, + }) + + await waitFor(() => { + expect(makeRequest.mock.calls.length > 0).toBe(true) + }) + + // "level" is not a default facet for this channel type, but it is requested + // as an aggregation because it is present in the URL. + const apiSearchParams = getLastApiSearchParams() + expect(apiSearchParams.getAll("aggregations")).toContain("level") + + // ...and it renders as a facet. + const facetsContainer = screen.getByTestId("facets-container") + await within(facetsContainer).findByText("Level") + }) + + test("Does not duplicate a facet already shown by default as an extra URL facet", async () => { + const { channel } = setMockApiResponses({ + channelPatch: { channel_type: ChannelTypeEnum.Unit }, + search: { + count: 700, + metadata: { + aggregations: { + topic: [{ key: "physics", doc_count: 100 }], + }, + suggestions: [], + }, + }, + }) + + // "topic" is a default facet for Unit channels and is also present in the + // URL; it must appear exactly once. + renderWithProviders(, { + url: `/c/${channel.channel_type}/${channel.name}/?topic=physics`, + }) + + const facetsContainer = await screen.findByTestId("facets-container") + expect(await within(facetsContainer).findAllByText("Topic")).toHaveLength(1) + }) + test("Submitting search text updates URL correctly", async () => { const resources = factories.learningResources.resources({ count: 10, diff --git a/frontends/main/src/app-pages/ChannelPage/ChannelSearch.tsx b/frontends/main/src/app-pages/ChannelPage/ChannelSearch.tsx index 149401d498..48a962b0e9 100644 --- a/frontends/main/src/app-pages/ChannelPage/ChannelSearch.tsx +++ b/frontends/main/src/app-pages/ChannelPage/ChannelSearch.tsx @@ -57,8 +57,20 @@ const ChannelSearch: React.FC = ({ const { facetNames, facetManifest } = useMemo( () => - getFacets(channelType, offerors, constantSearchParams, resourceTypeGroup), - [offerors, channelType, constantSearchParams, resourceTypeGroup], + getFacets( + channelType, + offerors, + constantSearchParams, + resourceTypeGroup, + searchParams, + ), + [ + offerors, + channelType, + constantSearchParams, + resourceTypeGroup, + searchParams, + ], ) const setPage = useCallback( diff --git a/frontends/main/src/app-pages/ChannelPage/searchRequests.ts b/frontends/main/src/app-pages/ChannelPage/searchRequests.ts index 66b4b45d59..bd158bff71 100644 --- a/frontends/main/src/app-pages/ChannelPage/searchRequests.ts +++ b/frontends/main/src/app-pages/ChannelPage/searchRequests.ts @@ -8,6 +8,7 @@ import type { import { LearningResourceOfferor } from "api" import { ChannelTypeEnum } from "api/v0" import { getFacetManifest } from "@/page-components/SearchDisplay/getFacetManifest" +import { getExtraFacetNames } from "@/app-pages/SearchPage/searchRequests" export const getConstantSearchParams = (searchFilter?: string) => { const searchParams: Facets & BooleanFacets = {} @@ -63,9 +64,16 @@ const getFacetManifestForChannelType = ( offerors: Record, constantSearchParams: Facets, resourceTypeGroup: string | null, + extraFacetNames: string[] = [], ): FacetManifest => { - const facets = FACETS_BY_CHANNEL_TYPE[channelType] || [] - return getFacetManifest(offerors, resourceTypeGroup) + // Facets shown by default for this channel type plus any additional facets + // present in the URL. Facets hard-coded by the channel config + // (constantSearchParams) are filtered out below. + const facets = [ + ...(FACETS_BY_CHANNEL_TYPE[channelType] || []), + ...extraFacetNames, + ] + return getFacetManifest(offerors, resourceTypeGroup, extraFacetNames) .filter( (facetSetting) => !Object.keys(constantSearchParams).includes(facetSetting.name) && @@ -81,12 +89,24 @@ export const getFacets = ( offerors: Record, constantSearchParams: Facets, resourceTypeGroup: string | null, + searchParams?: URLSearchParams, ) => { + // Facets already surfaced by the channel type or pinned by the channel config + // should not be duplicated as extra facets from the URL. + const baseFacetNames = [ + ...(FACETS_BY_CHANNEL_TYPE[channelType] || []), + ...Object.keys(constantSearchParams), + ] as UseResourceSearchParamsProps["facets"] + const extraFacetNames = searchParams + ? (getExtraFacetNames(searchParams, baseFacetNames) ?? []) + : [] + const facetManifest = getFacetManifestForChannelType( channelType, offerors, constantSearchParams, resourceTypeGroup, + extraFacetNames, ) const facetNames = Array.from( diff --git a/frontends/main/src/app/(site)/c/[channelType]/[name]/page.tsx b/frontends/main/src/app/(site)/c/[channelType]/[name]/page.tsx index 4d5abb4557..3eda8fe42f 100644 --- a/frontends/main/src/app/(site)/c/[channelType]/[name]/page.tsx +++ b/frontends/main/src/app/(site)/c/[channelType]/[name]/page.tsx @@ -91,11 +91,20 @@ const Page: React.FC> = async ({ const constantSearchParams = getConstantSearchParams(channel.search_filter) + const urlParams = new URLSearchParams( + Object.entries(search).flatMap(([key, value]) => + Array.isArray(value) + ? value.map((v) => [key, v]) + : [[key, String(value)]], + ), + ) + const { facetNames } = getFacets( channelType, offerors as unknown as Record, constantSearchParams, null, + urlParams, ) const searchRequest = getSearchParams({ From 0d32b4f92c057e2778a48e13a56228ee88e55a37 Mon Sep 17 00:00:00 2001 From: Shankar Ambady Date: Tue, 4 Aug 2026 16:24:00 -0400 Subject: [PATCH 3/8] =?UTF-8?q?added=20a=20fallback=20to=20populate=20run?= =?UTF-8?q?=20readable=20ids=20for=20contentfiles=20withou=E2=80=A6=20(#37?= =?UTF-8?q?19)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * added a fallback to populate run readable ids for contentfiles without runs * scope filter better --- vector_search/utils.py | 64 ++++++++++++++---- vector_search/utils_test.py | 127 ++++++++++++++++++++++++++++++++++++ 2 files changed, 177 insertions(+), 14 deletions(-) diff --git a/vector_search/utils.py b/vector_search/utils.py index 750aab6f27..093224e1fb 100644 --- a/vector_search/utils.py +++ b/vector_search/utils.py @@ -502,6 +502,27 @@ def _content_file_embedding_context(document): return document.get("content", "") +def _with_run_readable_id_fallback(serialized_document): + """ + Return the document with run_readable_id defaulted to the resource + readable_id for run-less content files (e.g. scraped marketing pages). + + The content-file search API rewrites resource_readable_id filters into + run_readable_id filters (see ContentFilesVectorSearchView), so every point + payload must carry a run_readable_id to stay reachable -- course-metadata + points already follow this convention. Only the Qdrant payload gets the + fallback; point ids still derive from the raw serialized document. + """ + if serialized_document.get("run_readable_id") or not serialized_document.get( + "resource_readable_id" + ): + return serialized_document + return { + **serialized_document, + "run_readable_id": serialized_document["resource_readable_id"], + } + + def _process_resource_embeddings(serialized_resources): docs = [] metadata = [] @@ -559,7 +580,7 @@ def update_content_file_payload(serialized_document): _set_payload( points, - serialized_document, + _with_run_readable_id_fallback(serialized_document), param_map=QDRANT_CONTENT_FILE_PARAM_MAP, collection_name=CONTENT_FILES_COLLECTION_NAME, ) @@ -847,16 +868,18 @@ def _generate_content_file_points(serialized_content, stored_payloads): break chunk_id, split_doc = valid_chunks[relative_index] - metadata = { - "resource_point_id": str(resource_vector_point_id), - "chunk_number": chunk_id, - "chunk_content": split_doc.page_content, - **{ - key: split_doc.metadata[key] - for key in QDRANT_CONTENT_FILE_PARAM_MAP - if key in split_doc.metadata - }, - } + metadata = _with_run_readable_id_fallback( + { + "resource_point_id": str(resource_vector_point_id), + "chunk_number": chunk_id, + "chunk_content": split_doc.page_content, + **{ + key: split_doc.metadata[key] + for key in QDRANT_CONTENT_FILE_PARAM_MAP + if key in split_doc.metadata + }, + } + ) point_id = vector_point_id( vector_point_key( @@ -1184,15 +1207,28 @@ def _content_file_vector_hits(search_result): keys = [hit.payload.get("key") for hit in search_result] serialized_content_files = ContentFileSerializer( - ContentFile.objects.for_serialization().filter( - run__run_id__in=run_readable_ids, key__in=keys + ContentFile.objects.for_serialization() + .filter(key__in=keys) + .filter( + Q(run__run_id__in=run_readable_ids) + | Q(run__isnull=True, learning_resource__readable_id__in=run_readable_ids) ), many=True, ).data results = [] contentfiles_dict = {} + # Run-less content files (e.g. marketing pages) serialize without a + # run_readable_id; their Qdrant payloads carry the resource readable_id + # in that field instead, so key them the same way here. [ - contentfiles_dict.update({(cf["run_readable_id"], cf["key"]): cf}) + contentfiles_dict.update( + { + ( + cf.get("run_readable_id") or cf.get("resource_readable_id"), + cf["key"], + ): cf + } + ) for cf in serialized_content_files ] results = [] diff --git a/vector_search/utils_test.py b/vector_search/utils_test.py index 37ff207cc7..041b4bed51 100644 --- a/vector_search/utils_test.py +++ b/vector_search/utils_test.py @@ -61,6 +61,7 @@ from vector_search.utils import ( _chunk_documents, _chunk_markdown_documents, + _content_file_vector_hits, _embed_course_metadata_as_contentfile, _generate_content_file_points, _get_text_splitter, @@ -1270,6 +1271,132 @@ def test_update_payload_no_points(mocker): mock_qdrant.set_payload.assert_not_called() +def test_generate_content_points_runless_run_readable_id_fallback(mocker): + """ + Run-less content files (e.g. scraped marketing pages) must get a + run_readable_id payload equal to the resource readable_id: the content-file + search API rewrites resource_readable_id filters into run_readable_id + filters, so points without the field are unreachable. Files with a real + run keep the run's id. + """ + settings.CONTENT_FILE_EMBEDDING_CHUNK_SIZE_OVERRIDE = 500 + settings.CONTENT_FILE_EMBEDDING_CHUNK_OVERLAP = 50 + mocker.patch("vector_search.utils.remove_points_matching_params") + mock_dense = mocker.MagicMock() + mock_dense.embed_documents.side_effect = lambda texts: [[0.1] for _ in texts] + mock_dense.model_short_name.return_value = "dense" + mock_sparse = mocker.MagicMock() + mock_sparse.embed_documents.side_effect = lambda texts: [[0.2] for _ in texts] + mock_sparse.model_short_name.return_value = "sparse" + mocker.patch("vector_search.utils.dense_encoder", return_value=mock_dense) + mocker.patch("vector_search.utils.sparse_encoder", return_value=mock_sparse) + + runless_doc = { + "content": "# Marketing page\n\nSome marketing content", + "file_type": "marketing_page", + "file_extension": ".md", + "platform": {"code": "xpro"}, + "resource_readable_id": "program-v1:xPRO+Test", + "key": "https://xpro.mit.edu/programs/program-v1:xPRO+Test/", + "checksum": "abc", + } + run_doc = { + "content": "Some plain text content", + "file_type": "page", + "file_extension": ".html", + "platform": {"code": "x"}, + "resource_readable_id": "r1", + "run_readable_id": "run1", + "key": "k1", + "checksum": "def", + } + + points = list(_generate_content_file_points([runless_doc, run_doc], {})) + + runless_payloads = [ + point.payload for point in points if point.payload["key"] == runless_doc["key"] + ] + run_payloads = [point.payload for point in points if point.payload["key"] == "k1"] + assert runless_payloads + assert run_payloads + assert all( + payload["run_readable_id"] == "program-v1:xPRO+Test" + for payload in runless_payloads + ) + assert all(payload["run_readable_id"] == "run1" for payload in run_payloads) + + +def test_update_payload_content_file_runless_backfills_run_readable_id(mocker): + """ + Payload refresh for a run-less content file writes the resource readable_id + into run_readable_id (healing pre-fix points without re-embedding), while + the point lookup keeps using the raw document so points stored without the + field are still found. + """ + resource = LearningResourceFactory.create(is_program=True) + content_file = ContentFileFactory.create( + learning_resource=resource, content="Test content" + ) + serialized = next(iter(serialize_bulk_content_files([content_file.id]))) + assert "run_readable_id" not in serialized + + mock_qdrant = mocker.MagicMock() + mocker.patch("vector_search.utils.qdrant_client", return_value=mock_qdrant) + mock_point = mocker.MagicMock() + mock_point.id = "test-point-id" + retrieve_mock = mocker.patch( + "vector_search.utils.retrieve_points_matching_params", return_value=[mock_point] + ) + + update_content_file_payload(serialized) + + assert "run_readable_id" not in retrieve_mock.call_args[0][0] + payload = mock_qdrant.set_payload.call_args[1]["payload"] + assert payload["run_readable_id"] == resource.readable_id + + +def test_content_file_vector_hits_hydrates_runless_files(): + """ + Search hits for run-less content files (e.g. marketing pages) are hydrated + with the serialized DB record, matched via the resource readable_id their + payloads carry in run_readable_id. + """ + resource = LearningResourceFactory.create(is_program=True) + content_file = ContentFileFactory.create( + learning_resource=resource, + content="marketing content", + file_type="marketing_page", + ) + run_content_file = ContentFileFactory.create(content="run file content") + hits = [ + PointStruct( + id=1, + payload={ + "run_readable_id": resource.readable_id, + "key": content_file.key, + "chunk_content": "marketing content", + }, + vector=[], + ), + PointStruct( + id=2, + payload={ + "run_readable_id": run_content_file.run.run_id, + "key": run_content_file.key, + "chunk_content": "run file content", + }, + vector=[], + ), + ] + + results = _content_file_vector_hits(hits) + + assert results[0]["id"] == content_file.id + assert results[0]["resource_readable_id"] == resource.readable_id + assert "content" not in results[0] + assert results[1]["id"] == run_content_file.id + + @pytest.mark.django_db def test_embed_learning_resources_summarizes_only_contentfiles_with_summary(mocker): """ From 3d669aeff2acc86809441cb548407013fec18ed1 Mon Sep 17 00:00:00 2001 From: Ahtesham Quraish Date: Wed, 5 Aug 2026 17:37:06 +0500 Subject: [PATCH 4/8] =?UTF-8?q?refactor:=20V1=20=E2=80=94=20Clean=20up=20a?= =?UTF-8?q?nd=20de-duplicate=20the=20VideoPlaylistCollectionPage=20folder?= =?UTF-8?q?=20(#3706)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Ahtesham Quraish --- .../FeaturedVideo.tsx | 15 +- .../MoreFromPlaylist.tsx | 134 ++++++ .../ShareDialog.tsx | 2 - .../VideoPlaylistCollectionPage/VideoCard.tsx | 49 +- .../VideoDetailPage.styled.ts | 234 +++++++++ .../VideoDetailPage.tsx | 452 ++---------------- .../VideoJsPlayer.tsx | 4 - .../VideoResourcePlayer.tsx | 14 +- .../VideoSeriesDetailPage.styled.ts | 33 +- .../VideoSeriesDetailPage.tsx | 9 +- .../VideoShareButton.tsx | 14 +- .../VideoShareDialog.tsx | 2 - .../shared.styled.ts | 102 ++++ .../ShareDialog}/ShareDialog.test.tsx | 10 +- .../ResourceCard/ResourceCard.test.tsx | 9 - 15 files changed, 546 insertions(+), 537 deletions(-) create mode 100644 frontends/main/src/app-pages/VideoPlaylistCollectionPage/MoreFromPlaylist.tsx delete mode 100644 frontends/main/src/app-pages/VideoPlaylistCollectionPage/ShareDialog.tsx create mode 100644 frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoDetailPage.styled.ts delete mode 100644 frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoShareDialog.tsx rename frontends/main/src/{app-pages/VideoPlaylistCollectionPage => components/ShareDialog}/ShareDialog.test.tsx (98%) diff --git a/frontends/main/src/app-pages/VideoPlaylistCollectionPage/FeaturedVideo.tsx b/frontends/main/src/app-pages/VideoPlaylistCollectionPage/FeaturedVideo.tsx index 7e2f62a318..5cea5f2bbd 100644 --- a/frontends/main/src/app-pages/VideoPlaylistCollectionPage/FeaturedVideo.tsx +++ b/frontends/main/src/app-pages/VideoPlaylistCollectionPage/FeaturedVideo.tsx @@ -7,6 +7,7 @@ import VideoContainer from "./VideoContainer" import { RiPlayFill } from "@remixicon/react" import { formatDurationClockTime } from "ol-utilities" import type { VideoResource } from "api/v1" +import { DurationBadge } from "./shared.styled" const PLACEHOLDER_IMG = "/images/mit-open-learning-logo.svg" @@ -56,6 +57,8 @@ const ImageWrapper = styled(Link, { }), })) +// Distinct from the shared `PlayOverlay`: this one is always visible and scales +// on hover (see ImageWrapper above) rather than fading a scrim in. const PlayOverlay = styled.div({ position: "absolute", inset: 0, @@ -76,18 +79,6 @@ const PlayCircle = styled.div({ justifyContent: "center", }) -const DurationBadge = styled.span(({ theme }) => ({ - ...theme.typography.body3, - position: "absolute", - bottom: 0, - right: 0, - backgroundColor: theme.custom.colors.darkGray2, - color: "#fff", - fontWeight: theme.typography.fontWeightMedium, - padding: "8px", - zIndex: 1, -})) - const TextSide = styled.div(({ theme }) => ({ [theme.breakpoints.down("sm")]: { padding: "16px 0 0", diff --git a/frontends/main/src/app-pages/VideoPlaylistCollectionPage/MoreFromPlaylist.tsx b/frontends/main/src/app-pages/VideoPlaylistCollectionPage/MoreFromPlaylist.tsx new file mode 100644 index 0000000000..8a51b0dfc8 --- /dev/null +++ b/frontends/main/src/app-pages/VideoPlaylistCollectionPage/MoreFromPlaylist.tsx @@ -0,0 +1,134 @@ +import React from "react" +import Image from "next/image" +import { Skeleton } from "ol-components" +import { formatDurationClockTime } from "ol-utilities" +import type { VideoResource } from "api/v1" +import { videoDetailPageView, videoPlaylistPageView } from "@/common/urls" +import * as Styled from "./VideoDetailPage.styled" + +type MoreFromPlaylistProps = { + playlistId: number + /** Display name of the playlist, used in headings and labels. */ + playlistLabel: string + playlistTitle?: string + /** Sibling videos to list, already filtered and capped by the caller. */ + videos: VideoResource[] + /** Total videos in the playlist, used to decide whether to link to the rest. */ + totalVideos: number + isLoading: boolean +} + +/** One row: thumbnail with duration badge and hover overlay, plus title/description. */ +const MoreFromPlaylistItem: React.FC<{ + video: VideoResource + playlistId: number +}> = ({ video, playlistId }) => { + const duration = video.video?.duration + ? formatDurationClockTime(video.video.duration) + : null + const imageUrl = video.image?.url ?? null + const topicNames = (video.topics ?? []) + .map((topic) => topic.name) + .filter(Boolean) + .join(" · ") + + return ( + + + {imageUrl && ( + {`Video + )} + {duration && ( + {duration} + )} + + + + + + + {video.title} + + {video.description && ( + + )} + + + ) +} + +/** + * "More from " — the sibling-video list at the foot of the video detail + * page. Renders nothing once loaded if the playlist has no other videos. + */ +const MoreFromPlaylist: React.FC = ({ + playlistId, + playlistLabel, + playlistTitle, + videos, + totalVideos, + isLoading, +}) => { + if (isLoading) { + return ( + <> + + {Array.from({ length: 3 }).map((_, i) => ( + + +
+ + +
+ + ))} + + ) + } + + if (videos.length === 0) return null + + // +1 accounts for the video currently being watched, which is excluded from + // `videos` — without it a fully-listed playlist would still offer "View all". + const hasMore = totalVideos > videos.length + 1 + + return ( + <> + More from {playlistLabel} + + {videos.map((video) => ( + + + + + ))} + + {hasMore && ( + + View all in {playlistLabel} → + + )} + + ) +} + +export default MoreFromPlaylist diff --git a/frontends/main/src/app-pages/VideoPlaylistCollectionPage/ShareDialog.tsx b/frontends/main/src/app-pages/VideoPlaylistCollectionPage/ShareDialog.tsx deleted file mode 100644 index 25acfe8b76..0000000000 --- a/frontends/main/src/app-pages/VideoPlaylistCollectionPage/ShareDialog.tsx +++ /dev/null @@ -1,2 +0,0 @@ -export { default } from "@/components/ShareDialog/ShareDialog" -export type { ShareDialogProps } from "@/components/ShareDialog/ShareDialog" diff --git a/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoCard.tsx b/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoCard.tsx index 6e60a03ce7..96510acf63 100644 --- a/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoCard.tsx +++ b/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoCard.tsx @@ -3,8 +3,13 @@ import Image from "next/image" import Link from "next/link" import { Typography, styled, theme, Skeleton } from "ol-components" import { formatDurationClockTime } from "ol-utilities" -import { RiPlayCircleFill } from "@remixicon/react" import type { VideoResource } from "api/v1" +import { + DurationBadge, + PlayOverlay, + PlayIcon, + ThumbnailWrapper, +} from "./shared.styled" const PLACEHOLDER_IMG = "/images/mit-open-learning-logo.svg" @@ -35,19 +40,6 @@ const VideoCardItem = styled(Link)({ }, }) -const ThumbnailWrapper = styled.div({ - position: "relative", - flexShrink: 0, - width: 160, - aspectRatio: "16/9", - overflow: "hidden", - backgroundColor: theme.custom.colors.black, - - [theme.breakpoints.down("sm")]: { - width: "100%", - }, -}) - const ThumbnailImage = styled(Image)(({ theme }) => ({ objectFit: "cover", width: "160px", @@ -57,30 +49,6 @@ const ThumbnailImage = styled(Image)(({ theme }) => ({ }, })) -const DurationBadge = styled.span(({ theme }) => ({ - ...theme.typography.body3, - position: "absolute", - bottom: 0, - right: 0, - backgroundColor: theme.custom.colors.darkGray2, - color: "#fff", - fontWeight: theme.typography.fontWeightMedium, - padding: "8px", - zIndex: 1, -})) - -const PlayOverlay = styled.div({ - position: "absolute", - inset: 0, - display: "flex", - alignItems: "center", - justifyContent: "center", - color: "#fff", - opacity: 0, - transition: "opacity 0.2s", - backgroundColor: "rgba(0, 0, 0, 0.18)", -}) - const CardContent = styled.div({ flex: 1, display: "flex", @@ -107,11 +75,6 @@ const CardTitle = styled(Typography)(({ theme }) => ({ }, })) -const PlayIcon = styled(RiPlayCircleFill)({ - width: 36, - height: 36, -}) - const CardMetaRow = styled.div({ display: "flex", alignItems: "flex-start", diff --git a/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoDetailPage.styled.ts b/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoDetailPage.styled.ts new file mode 100644 index 0000000000..d68df260c0 --- /dev/null +++ b/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoDetailPage.styled.ts @@ -0,0 +1,234 @@ +import Link from "next/link" +import { Typography, styled, theme } from "ol-components" +import VideoResourcePlayer from "./VideoResourcePlayer" + +// Primitives shared with the other video pages, re-exported so consumers of this +// module have a single import for the page's styles. +export { + SkipLinksNav, + StyledBreadcrumbs, + ScreenReaderOnly, + DurationBadge, + PlayOverlay, + PlayIcon, + ThumbnailWrapper, + VideoTitle, +} from "./shared.styled" + +// ── Page shell ── + +export const PageWrapper = styled.div({ + backgroundColor: "#fff", + minHeight: "100vh", +}) + +export const BreadcrumbBar = styled.div(({ theme }) => ({ + padding: "18px 0 2px 0", + borderBottom: `1px solid ${theme.custom.colors.red}`, + [theme.breakpoints.down("sm")]: { + padding: "12px 0 0 0", + }, +})) + +export const ContentArea = styled.div(({ theme }) => ({ + padding: "56px 0 80px", + [theme.breakpoints.down("sm")]: { + padding: "32px 0 80px", + }, +})) + +export const CategoryLabel = styled(Link)(({ theme }) => ({ + display: "block", + ...theme.typography.body3, + fontWeight: theme.typography.fontWeightBold, + color: theme.custom.colors.red, + textTransform: "uppercase", + letterSpacing: "1.92px", + marginBottom: "8px", + fontSize: "12px", + fontStyle: "normal", + lineHeight: "150%" /* 18px */, + "&:hover": { + textDecoration: "underline", + }, +})) + +// ── Title / meta row ── + +export const VideoShareSection = styled("div")({ + display: "flex", + alignItems: "center", + justifyContent: "space-between", + flexWrap: "wrap", + gap: "8px", + marginBottom: "24px", + [theme.breakpoints.down("sm")]: { + marginBottom: "16px", + }, +}) + +export const MetaRow = styled.div({ + ...theme.typography.body2, + color: theme.custom.colors.darkGray1, +}) + +export const TopicText = styled.span(({ theme }) => ({ + color: theme.custom.colors.silverGrayDark, + ...theme.typography.body2, + lineHeight: "22px", + paddingLeft: "8px", +})) + +export const DurationText = styled.span(({ theme }) => ({ + color: theme.custom.colors.black, + ...theme.typography.body2, + lineHeight: "22px", + fontWeight: theme.typography.fontWeightBold, +})) + +// ── Player / description ── + +export const StyledVideoResourcePlayer = styled(VideoResourcePlayer)( + ({ theme }) => ({ + [theme.breakpoints.down("sm")]: { + marginTop: "0", + }, + }), +) + +export const BorderLine = styled.div(({ theme }) => ({ + borderBottom: `4px solid ${theme.custom.colors.darkGray2}`, + marginBottom: "40px", + [theme.breakpoints.down("sm")]: { + marginBottom: "24px", + }, +})) + +export const DescriptionText = styled(Typography)(({ theme }) => ({ + ...theme.typography.body1, + color: theme.custom.colors.darkGray2, + marginBottom: "22px", + fontSize: "18px", + fontWeight: theme.typography.fontWeightMedium, + lineHeight: "30px", + [theme.breakpoints.down("sm")]: { + fontSize: "16px", + lineHeight: "28px", + marginBottom: "24px", + }, +})) + +// ── "More from playlist" list ── + +export const MoreFromTitle = styled(Typography)(({ theme }) => ({ + ...theme.typography.body3, + fontWeight: theme.typography.fontWeightBold, + textTransform: "uppercase", + color: theme.custom.colors.black, + padding: "32px 0", + lineHeight: "150%", + letterSpacing: "1.92px", + [theme.breakpoints.down("sm")]: { + padding: "24px 0", + }, +})) + +export const MoreFromList = styled.div({ + display: "flex", + flexDirection: "column", +}) + +export const MoreFromItem = styled(Link)({ + display: "flex", + alignItems: "flex-start", + gap: "24px", + padding: "24px 0", + borderBottom: `1px solid ${theme.custom.colors.lightGray2}`, + textDecoration: "none", + "&:hover .mf-title": { color: theme.custom.colors.red }, + + "&:hover .video-card-title, &:focus-visible .video-card-title": { + color: theme.custom.colors.red, + }, + + "&:hover .play-overlay": { + opacity: 0.5, + }, + + "&:focus-visible .play-overlay": { + opacity: 0.5, + }, + + "&:first-child": { + padding: "0 0 24px 0", + }, + + [theme.breakpoints.down("sm")]: { + flexDirection: "column", + }, +}) + +export const MoreFromTextSide = styled.div(({ theme }) => ({ + flex: 1, + minWidth: 0, + paddingTop: "17px", + [theme.breakpoints.down("sm")]: { + paddingTop: 0, + }, +})) + +export const MoreFromItemTitle = styled(Typography)({ + ...theme.typography.subtitle2, + fontWeight: theme.typography.fontWeightBold, + color: theme.custom.colors.black, + transition: "color 0.15s", + marginBottom: "4px", + fontSize: "20px", + lineHeight: "26px" /* 130% */, +}) + +export const MoreFromItemMeta = styled(Typography)({ + ...theme.typography.body2, + color: theme.custom.colors.silverGrayDark, + overflow: "hidden", + display: "-webkit-box", + WebkitLineClamp: 2, + WebkitBoxOrient: "vertical", + lineHeight: "22px", +}) + +export const SeeAllLink = styled(Link)(({ theme }) => ({ + display: "inline-flex", + alignItems: "center", + marginTop: "40px", + ...theme.typography.body1, + color: theme.custom.colors.red, + fontWeight: theme.typography.fontWeightMedium, + lineHeight: "150%", + textDecoration: "none", + "&:hover": { textDecoration: "underline" }, + [theme.breakpoints.down("sm")]: { + marginTop: "28px", + }, +})) + +/** Mobile-only gap between "More from" rows; collapses to nothing on desktop. */ +export const SpacerBlock = styled.div(({ theme }) => ({ + "& .spacer-block": { + display: "none", + }, + [theme.breakpoints.down("sm")]: { + height: "8px", + "& .spacer-block": { + display: "block", + }, + }, +})) + +/** Row placeholder matching a "More from" item while the playlist loads. */ +export const MoreFromSkeletonRow = styled.div(({ theme }) => ({ + display: "flex", + gap: 16, + padding: "16px 0", + borderBottom: `1px solid ${theme.custom.colors.lightGray2}`, +})) diff --git a/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoDetailPage.tsx b/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoDetailPage.tsx index 4c3b1661d3..84e73c2518 100644 --- a/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoDetailPage.tsx +++ b/frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoDetailPage.tsx @@ -2,13 +2,10 @@ import { env } from "@/env" import React, { useEffect, useRef } from "react" -import Link from "next/link" -import Image from "next/image" -import { Typography, styled, theme, Skeleton, SkipLink } from "ol-components" +import { Skeleton, SkipLink } from "ol-components" import VideoContainer from "./VideoContainer" -import { RiPlayCircleFill } from "@remixicon/react" -import { SkipLinksNav, StyledBreadcrumbs } from "./shared.styled" import VideoShareButton from "./VideoShareButton" +import MoreFromPlaylist from "./MoreFromPlaylist" import { useQuery } from "@tanstack/react-query" import { useLearningResourcesDetail, @@ -19,284 +16,13 @@ import { VideoResourceResourceTypeEnum } from "api/v1" import { formatDurationClockTime } from "ol-utilities" import { videoDetailPageView, videoPlaylistPageView } from "@/common/urls" import { buildVideoStructuredData } from "./videoStructuredData" -import VideoResourcePlayer from "./VideoResourcePlayer" import type { VideoPlayerHandle } from "./VideoResourcePlayer" +import * as Styled from "./VideoDetailPage.styled" const NEXT_PUBLIC_ORIGIN = env("NEXT_PUBLIC_ORIGIN") -const PageWrapper = styled.div({ - backgroundColor: "#fff", - minHeight: "100vh", -}) - -const BreadcrumbBar = styled.div(({ theme }) => ({ - padding: "18px 0 2px 0", - borderBottom: `1px solid ${theme.custom.colors.red}`, - [theme.breakpoints.down("sm")]: { - padding: "12px 0 0 0", - }, -})) - -const PlayIcon = styled(RiPlayCircleFill)({ - width: 36, - height: 36, -}) - -const ContentArea = styled.div(({ theme }) => ({ - padding: "56px 0 80px", - [theme.breakpoints.down("sm")]: { - padding: "32px 0 80px", - }, -})) - -const CategoryLabel = styled(Link)(({ theme }) => ({ - display: "block", - ...theme.typography.body3, - fontWeight: theme.typography.fontWeightBold, - color: theme.custom.colors.red, - textTransform: "uppercase", - letterSpacing: "1.92px", - marginBottom: "8px", - fontSize: "12px", - fontStyle: "normal", - lineHeight: "150%" /* 18px */, - "&:hover": { - textDecoration: "underline", - }, -})) - -const StyledVideoShareButton = styled(VideoShareButton)({ - height: "40px", - padding: "18px 12px", -}) - -const VideoShareSection = styled("div")({ - display: "flex", - alignItems: "center", - justifyContent: "space-between", - flexWrap: "wrap", - gap: "8px", - marginBottom: "24px", - [theme.breakpoints.down("sm")]: { - marginBottom: "16px", - }, -}) - -const VideoTitle = styled.h1(({ theme }) => ({ - ...theme.typography.h2, - fontWeight: theme.typography.fontWeightBold, - color: theme.custom.colors.black, - margin: "0 0 24px", - "&:focus": { outline: "none" }, - fontSize: "44px", - fontStyle: "normal", - lineHeight: "120%" /* 52.8px */, - letterSpacing: "-0.88px", - [theme.breakpoints.down("sm")]: { - ...theme.typography.h3, - margin: "0 0 14px", - letterSpacing: "inherit", - }, -})) - -const MetaRow = styled.div({ - ...theme.typography.body2, - color: theme.custom.colors.darkGray1, -}) - -const StyledVideoResourcePlayer = styled(VideoResourcePlayer)(({ theme }) => ({ - [theme.breakpoints.down("sm")]: { - marginTop: "0", - }, -})) - -const DescriptionText = styled(Typography)(({ theme }) => ({ - ...theme.typography.body1, - color: theme.custom.colors.darkGray2, - marginBottom: "22px", - fontSize: "18px", - fontWeight: theme.typography.fontWeightMedium, - lineHeight: "30px", - [theme.breakpoints.down("sm")]: { - fontSize: "16px", - lineHeight: "28px", - marginBottom: "24px", - }, -})) - -const MoreFromTitle = styled(Typography)(({ theme }) => ({ - ...theme.typography.body3, - fontWeight: theme.typography.fontWeightBold, - textTransform: "uppercase", - color: theme.custom.colors.black, - padding: "32px 0", - lineHeight: "150%", - letterSpacing: "1.92px", - [theme.breakpoints.down("sm")]: { - padding: "24px 0", - }, -})) - -const MoreFromList = styled.div({ - display: "flex", - flexDirection: "column", -}) - -const BorderLine = styled.div(({ theme }) => ({ - borderBottom: `4px solid ${theme.custom.colors.darkGray2}`, - marginBottom: "40px", - [theme.breakpoints.down("sm")]: { - marginBottom: "24px", - }, -})) - -const MoreFromItem = styled(Link)({ - display: "flex", - alignItems: "flex-start", - gap: "24px", - padding: "24px 0", - borderBottom: `1px solid ${theme.custom.colors.lightGray2}`, - textDecoration: "none", - "&:hover .mf-title": { color: theme.custom.colors.red }, - - "&:hover .video-card-title, &:focus-visible .video-card-title": { - color: theme.custom.colors.red, - }, - - "&:hover .play-overlay": { - opacity: 0.5, - }, - - "&:focus-visible .play-overlay": { - opacity: 0.5, - }, - - "&:first-child": { - padding: "0 0 24px 0", - }, - - [theme.breakpoints.down("sm")]: { - flexDirection: "column", - }, -}) - -const MoreFromThumbnailWrapper = styled.div(({ theme }) => ({ - position: "relative", - flexShrink: 0, - width: 160, - aspectRatio: "16/9", - backgroundColor: theme.custom.colors.black, - overflow: "hidden", - [theme.breakpoints.down("sm")]: { - width: "100%", - }, -})) - -const DurationBadge = styled.span(({ theme }) => ({ - ...theme.typography.body3, - position: "absolute", - bottom: 0, - right: 0, - backgroundColor: theme.custom.colors.darkGray2, - color: "#fff", - fontWeight: theme.typography.fontWeightMedium, - padding: "4px 6px", - zIndex: 1, -})) - -const MoreFromTextSide = styled.div(({ theme }) => ({ - flex: 1, - minWidth: 0, - paddingTop: "17px", - [theme.breakpoints.down("sm")]: { - paddingTop: 0, - }, -})) - -const MoreFromItemTitle = styled(Typography)({ - ...theme.typography.subtitle2, - fontWeight: theme.typography.fontWeightBold, - color: theme.custom.colors.black, - transition: "color 0.15s", - marginBottom: "4px", - fontSize: "20px", - lineHeight: "26px" /* 130% */, -}) - -const MoreFromItemMeta = styled(Typography)({ - ...theme.typography.body2, - color: theme.custom.colors.silverGrayDark, - overflow: "hidden", - display: "-webkit-box", - WebkitLineClamp: 2, - WebkitBoxOrient: "vertical", - lineHeight: "22px", -}) - -const SeeAllLink = styled(Link)(({ theme }) => ({ - display: "inline-flex", - alignItems: "center", - marginTop: "40px", - ...theme.typography.body1, - color: theme.custom.colors.red, - fontWeight: theme.typography.fontWeightMedium, - lineHeight: "150%", - textDecoration: "none", - "&:hover": { textDecoration: "underline" }, - [theme.breakpoints.down("sm")]: { - marginTop: "28px", - }, -})) - -const SpacerBlock = styled.div(({ theme }) => ({ - "& .spacer-block": { - display: "none", - }, - [theme.breakpoints.down("sm")]: { - height: "8px", - "& .spacer-block": { - display: "block", - }, - }, -})) - -const TopicText = styled.span(({ theme }) => ({ - color: theme.custom.colors.silverGrayDark, - ...theme.typography.body2, - lineHeight: "22px", - paddingLeft: "8px", -})) - -const DurationText = styled.span(({ theme }) => ({ - color: theme.custom.colors.black, - ...theme.typography.body2, - lineHeight: "22px", - fontWeight: theme.typography.fontWeightBold, -})) - -const PlayOverlay = styled.div({ - position: "absolute", - inset: 0, - display: "flex", - alignItems: "center", - justifyContent: "center", - color: "#fff", - opacity: 0, - transition: "opacity 0.2s", - backgroundColor: "rgba(0, 0, 0, 0.18)", -}) - -const ScreenReaderOnly = styled.span({ - position: "absolute", - width: 1, - height: 1, - padding: 0, - margin: -1, - overflow: "hidden", - clip: "rect(0, 0, 0, 0)", - whiteSpace: "nowrap", - border: 0, -}) +/** How many sibling videos the "More from" list shows at most. */ +const MORE_FROM_LIMIT = 5 type VideoDetailPageProps = { videoId: number @@ -343,7 +69,7 @@ const VideoDetailPage: React.FC = ({ item.resource_type === VideoResourceResourceTypeEnum.Video && item.id !== videoId, ) - .slice(0, 5) + .slice(0, MORE_FROM_LIMIT) const totalPlaylistVideos = (playlistItems ?? []).filter( (item) => item.resource_type === VideoResourceResourceTypeEnum.Video, @@ -375,7 +101,7 @@ const VideoDetailPage: React.FC = ({ const structuredData = !isLoading ? buildVideoStructuredData(video) : null return ( - + {structuredData && (