fix: link the podcast episode to its detail page and fix the video count on collection page#3345
fix: link the podcast episode to its detail page and fix the video count on collection page#3345ahtesham-quraish wants to merge 2 commits into
Conversation
…unt on collection page
OpenAPI Changes1 changes: 0 error, 1 warning, 0 info Unexpected changes? Ensure your branch is up-to-date with |
There was a problem hiding this comment.
Pull request overview
This PR updates the video playlist collection and podcast pages to (1) display a correct total video count for a playlist and (2) make podcast episode rows navigate to the episode detail page (while keeping play/pause on the play button).
Changes:
- Video playlist collection page now derives
totalVideosfrom the paginated responsecountinstead of the number of currently loaded items. - Podcast episode list items now accept an
hrefand navigate to episode detail on row click; play/pause is handled via the play button with propagation stopped. - Podcast episode detail page “More from …” list now links each episode to its own detail page.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| frontends/main/src/app-pages/VideoPlaylistCollectionPage/VideoPlaylistCollectionPage.tsx | Adjusts total video count logic and playlist page gating; passes updated totals to header/featured components. |
| frontends/main/src/app-pages/PodcastPage/PodcastEpisodeDetailPage.tsx | Adds episode detail linking for the “More from …” episode list. |
| frontends/main/src/app-pages/PodcastPage/PodcastDetailPage.tsx | Makes episode rows navigable via href and updates click handling to separate navigation vs play/pause. |
| limit: VIDEOS_PAGE_SIZE, | ||
| }) | ||
|
|
||
| console.log("itemsData", itemsData) |
| }) | ||
|
|
||
| if (!showVideoPlaylistPage) { | ||
| if (showVideoPlaylistPage) { |
| isSeries={playlistType} | ||
| totalVideos={totalVideos} | ||
| totalVideos={totalCount} | ||
| totalTime={totalTime} |
| })) | ||
|
|
||
| const EpisodeList = styled.ul({ | ||
| const EpisodeList = styled.div({ | ||
| listStyle: "none", | ||
| margin: 0, |
| onClick={() => (isPlaying ? onPauseClick?.() : onPlayClick(episode))} | ||
| isEpisodePage={isEpisodePage} | ||
| > | ||
| <EpisodeRow onClick={() => router.push(href)} isEpisodePage={isEpisodePage}> |
| {episodes.map((episode) => ( | ||
| <EpisodeItem | ||
| key={episode.id} | ||
| episode={episode} | ||
| href={ |
ahtesham-quraish
force-pushed
the
ahtesham/fix-video-count
branch
2 times, most recently
from
4e69fb5 to
0aeec2e
Compare
ahtesham-quraish
force-pushed
the
ahtesham/fix-video-count
branch
from
0aeec2e to
06ee46c
Compare
| handleRowNavigate() | ||
| } | ||
| }} | ||
| role="link" |
There was a problem hiding this comment.
role="link" semantically implies an href attribute (like an anchor). Screen readers will announce it as a link but there's no associated URL in the accessible tree, and AT-users can't copy/open-in-new-tab.
Instead render EpisodeRow as an tag passing href directly, and remove the manual role, tabIndex, onClick, and onKeyDown, the browser handles all of that natively.
| } | ||
| isPlayable={Boolean(getEpisodeAudioUrl(episode))} | ||
| /> | ||
| <div key={episode.id} role="listitem"> |
There was a problem hiding this comment.
EpisodeList is now a div role="list", and each item is wrapped in <div role="listitem"> containing <EpisodeItem> which itself renders EpisodeRow (another div). This adds an extra DOM node with no purpose.
if you change:
{episodes.map((episode) => (
<div key={episode.id} role="listitem">
<EpisodeItem
episode={episode}
href={...}
...
/>
</div>
))}
to
{episodes.map((episode) => (
<EpisodeItem
key={episode.id}
episode={episode}
href={...}
...
/>
))}
and add role="listitem" to EpisodeRow that eliminates the extra div wrapper with no semantic purpose
daniellefrappier18
left a comment
daniellefrappier18
left a comment
There was a problem hiding this comment.
This looks good but I think we should fix the role="link" on a div without href which is an invalid ARIA pattern before merging
What are the relevant tickets?
n/a
Description (What does it do?)
Screenshots (if appropriate):
Screen.Recording.2026-05-15.at.4.43.20.PM.mov
How can this be tested?
if you don't have playlist data locally then in frontend env file you should add the following variable
NEXT_PUBLIC_MITOL_API_BASE_URL="https://api.rc.learn.mit.edu"it will connect with RC learn backendthen we need to enable the flag which is
podcast-detail-pageand then visit the following urlhttp://open.odl.local:8062/podcast/detail/15261?podcast=15260Additional Context