Skip to content

Stats: Link video list titles to single video page - #112494

Merged
kangzj merged 3 commits into
trunkfrom
stats-301-link-items-in-videos-list-page-to-single-video-page
Jul 10, 2026
Merged

Stats: Link video list titles to single video page#112494
kangzj merged 3 commits into
trunkfrom
stats-301-link-items-in-videos-list-page-to-single-video-page

Conversation

@adamwoodnz

@adamwoodnz adamwoodnz commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Fixes STATS-301

Why are these changes being made?

On the videos list page (/stats/day/videoplays/<site>), each video title linked to the media file/media/<site>/<id> in Calypso, upload.php?item=<id> in Odyssey. As part of the VideoPress stats modernization, the title should open the video's stats instead, consistent with the metric columns (Impressions, Hours Watched, Retention Rate, Views), which already link to the single-video details page.

Proposed Changes

  • Repoint the video title link from the media file to the single-video stats page: /stats/<period>/videodetails/<site>?post=<id> (no statType, so the details page opens on its default Views chart — matching the target URL in the issue).
  • Render the title as a real <a> anchor instead of a <span role="button">, so keyboard activation, screen-reader link semantics, and right-/cmd-/middle-click "open in new tab" all work. The click still routes in-app via page() and defers to the browser for modified clicks.
  • Remove the now-dead media-navigation helper (editVideo) and its Odyssey-only dependencies (config, getSiteAdminUrl, siteAdminUrl).
  • Adjust the module SCSS so the title anchor keeps its existing appearance (no default link underline/colour).

The metric-column links are intentionally left unchanged — they are out of scope for this issue.

Screenshots

Videos list — each title now links to the single-video stats page

videos-list-title-links

Testing Instructions

  1. Open /stats/day/videoplays/<site> for a site with VideoPress videos.
  2. Click a video title → you should land on that video's stats page (/stats/<period>/videodetails/<site>?post=<id>), not the media library.
  3. Confirm the metric columns (Impressions / Hours Watched / Retention Rate / Views) still open the same details page focused on their metric.
  4. Keyboard: Tab to a title and press Enter — it should navigate. Right-click → "Open in new tab" should also work.
  5. Confirm the title styling is unchanged (no blue/underlined link appearance).

Verified locally against a Jetpack site: all 58 title cells render as anchors to videodetails, none to /media/, and clicking navigates in-app to the details page (e.g. ?post=247801).

Pre-merge Checklist

  • Has the general commit checklist been followed? (PCYsg-hS-p2)
  • Have you written new tests for your changes?
  • Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
  • Have you checked for TypeScript, React or other console errors?
  • For UI changes, have you tested the affected components in dark mode?
  • Have you tested accessibility for your changes? Ensure the feature remains usable with various user agents (e.g., browsers), interfaces (e.g., keyboard navigation), and assistive technologies (e.g., screen readers) (PCYsg-S3g-p2).
  • Have you used memoizing on expensive computations?
  • Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
    • For UI changes, have we tested the change in various languages?
  • For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-aUh-p2)?

🤖 Generated with Claude Code

@adamwoodnz

Copy link
Copy Markdown
Contributor Author

@copilot review this PR. Leave your feedback as review comments only — do NOT push commits, apply suggestions, or modify the branch in any way. I will make all code changes myself.

This comment was marked as resolved.

@adamwoodnz
adamwoodnz force-pushed the stats-301-link-items-in-videos-list-page-to-single-video-page branch from 47abe60 to 000ab35 Compare July 10, 2026 01:42
@adamwoodnz

Copy link
Copy Markdown
Contributor Author

Thanks for the review — went through the three points:

1. Tracks event before the modified-click guard — this is deliberate. Firing before the guard means a Cmd/middle-click that opens the video details in a new tab is still counted as a "details clicked" interaction, which is the intent to view is what we want to measure, regardless of same-tab vs new-tab. Leaving it as-is.

2. :visited colour bleed — good catch, fixed in 000ab3573c2. Added &:visited { color: var(--color-text); } to the title anchor. Calypso has global anchor styling in the cascade, so an explicit override is the safe way to keep the title from ever picking up a visited-link colour.

3. data.period when data is null — agreed it's safe today (videoDetailsHref is only called while iterating completeVideoStats, which is empty when data is null) and matches the pre-existing showStat pattern. Leaving it as pre-existing; no change here.

@adamwoodnz

Copy link
Copy Markdown
Contributor Author

@copilot review this PR. Leave your feedback as review comments only — do NOT push commits, apply suggestions, or modify the branch in any way. I will make all code changes myself.

This comment was marked as resolved.

On the videos list page, each video title linked to the media file
(/media/... in Calypso, upload.php in Odyssey). As part of the
VideoPress stats modernization, the title should instead open the
video's stats, matching where the metric columns already point.

Repoint the title to /stats/<period>/videodetails/<site>?post=<id>
(no statType, so the details page opens on its default Views chart).

Render the title as a real anchor instead of a span with role=button
so keyboard activation, screen-reader link semantics, and
right/cmd/middle-click "open in new tab" all work. The now-unused
media-navigation helper and its Odyssey-only imports are removed.

Fixes STATS-301
Guard the title anchor against global a:visited author rules bleeding
the browser visited-link colour into the title text.
@adamwoodnz
adamwoodnz force-pushed the stats-301-link-items-in-videos-list-page-to-single-video-page branch from 000ab35 to 80aa8b0 Compare July 10, 2026 01:57
@adamwoodnz

Copy link
Copy Markdown
Contributor Author

Review cycle complete — handing off for final review

Copilot rounds: 2 (approved — "looks good to merge", no further actionable items).

Addressed:

  • Pinned the title anchor's :visited colour (&:visited { color: var(--color-text); }) so Calypso's global anchor styles can't bleed a visited-link colour into the title. (Copilot confirmed the row-hover rule still wins by specificity.)
  • Kept the tracks event firing before the modified-click guard — deliberate, so "open in new tab" is still counted as intent-to-view-details.
  • data.period null-safety: left as-is (only called while iterating a non-empty list; matches the pre-existing showStat pattern).

Branch: rebased on trunk each round; currently up to date.

CI: all GitHub checks green. Unit tests (Web app) (TeamCity) is red, but the analysis points to a flake unrelated to this change:

  • No test in the repo imports or renders the changed component (videopress-stats-module) — no snapshot, no summary-page render test — so removing imports from this one isolated file cannot break another test file.
  • The two adjacent stats test files (state/stats/lists/test/utils.js, my-sites/stats/test/index.js) pass locally — 128 tests.
  • Unit tests (Web app) is green on recent trunk.
  • This check runs on TeamCity, so it can't be re-run via gh. Recommend re-running the TeamCity build to confirm the flake.

Ready for your final review. Still a draft — mark ready when you're happy.

renderTitleCell took five positional args and this change reordered
them (onClick/onKeyUp became href/onClick). A single named-field
object removes the positional-order footgun for any future caller.
@adamwoodnz
adamwoodnz force-pushed the stats-301-link-items-in-videos-list-page-to-single-video-page branch from d20746f to 65e340d Compare July 10, 2026 02:21
@adamwoodnz
adamwoodnz marked this pull request as ready for review July 10, 2026 02:25
@adamwoodnz
adamwoodnz requested a review from a team July 10, 2026 02:46
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jul 10, 2026
@adamwoodnz adamwoodnz self-assigned this Jul 10, 2026

@kangzj kangzj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Works as described 👍

@kangzj
kangzj added this pull request to the merge queue Jul 10, 2026
Merged via the queue into trunk with commit 3f5fa1f Jul 10, 2026
14 checks passed
@kangzj
kangzj deleted the stats-301-link-items-in-videos-list-page-to-single-video-page branch July 10, 2026 05:30
@github-actions github-actions Bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jul 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants