WS-2117 Add lightweight contextual navigation attribution - #14228
Conversation
…ion-attribution
…ion-attribution
…on' of github.com:bbc/simorgh into WS-2117-add-lightweight-contextual-navigation-attribution
…ion-attribution
There was a problem hiding this comment.
Pull request overview
This PR updates Simorgh’s “new” header navigation behaviour so the highlighted top nav item is context-aware (Watch/Listen for relevant media pages) and improves screen reader announcements by only using “current page” when the highlight reflects a genuine location.
Changes:
- Added
primaryMediaTypedetection inPageLayoutWrapper(viablockTypes/ content blocks) and threaded it throughHeaderContainerinto the newNavigationcontainer. - Reworked top-nav active-item selection in
Navigation/index.tsxto use URL matching first, then page-type/media-type attribution, plus ashouldAnnounceflag to control “current page” announcements. - Updated
psammead-navigationlink rendering so active styling is independent of whether “current page” is announced, and added extensive navigation attribution tests/snapshots.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| ws-nextjs-app/integration/pages/live/arabic/snapshots/canonical.test.ts.snap | Snapshot update for live Arabic integration test output. |
| src/app/legacy/psammead/psammead-navigation/src/index.jsx | Adds announceCurrentPage control and decouples active underline styling from screen reader “current page” announcement. |
| src/app/legacy/containers/Header/index.jsx | Passes primaryMediaType down into the new navigation container. |
| src/app/components/PageLayoutWrapper/index.tsx | Detects primaryMediaType from page data and provides it to the header/navigation. |
| src/app/components/Navigation/index.tsx | Implements page-type/media-type based top-nav attribution and introduces shouldAnnounce behaviour for a11y. |
| src/app/components/Navigation/index.test.tsx | Adds broad unit coverage for the new attribution/announcement rules. |
| src/app/components/Navigation/snapshots/index.test.tsx.snap | Updates snapshots to reflect the new active/announcement markup. |
…ion-attribution
…ion-attribution
| // Watch/Listen category attribution. False when Home is highlighted only as | ||
| // a last-resort default (no match, or categorized Watch/Listen doesn't exist | ||
| // in nav so falls back to Home). The bottom nav only marks items active on | ||
| // genuine URL matches and always announces. The dropdown nav never marks |
There was a problem hiding this comment.
Maybe i've misunderstood but does this match the acceptance criteria? The ticket says secondary navigation items should not be highlighted, but this says matching bottom navigation items remain active and are announced
There was a problem hiding this comment.
I think the ticket might be wrong there but will check. I would have to remove current functionality, that we only recently added, to fulfil that criteria!
There was a problem hiding this comment.
Fine to keep the sub-navigation functionality that already existed 👍
|
Tested with extended cache on preview. See slack thread for screenshots: https://bbc-tpg.slack.com/archives/C03RK72084S/p1787152060510699 |
|
|
||
| const { metadata: { atiAnalytics = undefined } = {} } = pageData ?? {}; | ||
|
|
||
| if (typeof window !== 'undefined') { |
There was a problem hiding this comment.
Is this log meant to be removed when we revert the increased cache max-age?
There was a problem hiding this comment.
yes, I will remove the logs and cache change when we have finished testing 👍
|
|
||
| const primaryMediaType = (() => { | ||
| const blockTypes = pageData?.blockTypes ?? []; | ||
| if (blockTypes.includes('audio')) return 'audio' as const; | ||
| if (blockTypes.includes('video')) return 'video' as const; | ||
| // Fallback: scan top-level content blocks for an audio or video block. | ||
| // This covers SFV articles where blockTypes may not be populated. | ||
| const contentBlocks = (pageData?.content?.model?.blocks ?? []) as { | ||
| type: string; | ||
| }[]; | ||
| if (contentBlocks.some(b => b.type === 'audio')) return 'audio' as const; | ||
| if (contentBlocks.some(b => b.type === 'video')) return 'video' as const; | ||
| return undefined; | ||
| })(); |
There was a problem hiding this comment.
I appreciate the use of comments here as a guide.
Please consider adding tests as a way of documenting the scenario cover by this block of code.
There was a problem hiding this comment.
I have added tests for this block of code now 👍
| const watchIndex = topItems.findIndex(item => item.type === 'watch'); | ||
| const listenIndex = topItems.findIndex(item => item.type === 'listen'); |
There was a problem hiding this comment.
| const watchIndex = topItems.findIndex(item => item.type === 'watch'); | |
| const listenIndex = topItems.findIndex(item => item.type === 'listen'); | |
| const focusableItemIndex = topItems.findIndex(item => item.type === 'watch' || item.type === 'listen'); |
| // Video pages, video mediaArticles, and article pages with a video primaryMediaType. | ||
| // If there's no Watch item to attribute to, this falls back to highlighting | ||
| // Home as a default categorisation only, so it shouldn't be announced. | ||
| if ( | ||
| pageType === TV_PAGE || | ||
| pageType === LIVE_TV_PAGE || | ||
| (pageType === MEDIA_ARTICLE_PAGE && primaryMediaType === 'video') || | ||
| (pageType === ARTICLE_PAGE && primaryMediaType === 'video') | ||
| ) { | ||
| return watchIndex > -1 | ||
| ? { index: watchIndex, shouldAnnounce: true } | ||
| : { index: 0, shouldAnnounce: false }; | ||
| } | ||
|
|
||
| // Audio pages, audio mediaArticles, and article pages with an audio primaryMediaType | ||
| // If there's no Listen item to attribute to, this falls back to highlighting | ||
| // Home as a default categorisation only, so it shouldn't be announced. | ||
| if ( | ||
| pageType === AUDIO_PAGE || | ||
| pageType === LIVE_RADIO_PAGE || | ||
| (pageType === MEDIA_ARTICLE_PAGE && primaryMediaType === 'audio') || | ||
| (pageType === ARTICLE_PAGE && primaryMediaType === 'audio') | ||
| ) { | ||
| return listenIndex > -1 | ||
| ? { index: listenIndex, shouldAnnounce: true } | ||
| : { index: 0, shouldAnnounce: false }; | ||
| } |
There was a problem hiding this comment.
| // Video pages, video mediaArticles, and article pages with a video primaryMediaType. | |
| // If there's no Watch item to attribute to, this falls back to highlighting | |
| // Home as a default categorisation only, so it shouldn't be announced. | |
| if ( | |
| pageType === TV_PAGE || | |
| pageType === LIVE_TV_PAGE || | |
| (pageType === MEDIA_ARTICLE_PAGE && primaryMediaType === 'video') || | |
| (pageType === ARTICLE_PAGE && primaryMediaType === 'video') | |
| ) { | |
| return watchIndex > -1 | |
| ? { index: watchIndex, shouldAnnounce: true } | |
| : { index: 0, shouldAnnounce: false }; | |
| } | |
| // Audio pages, audio mediaArticles, and article pages with an audio primaryMediaType | |
| // If there's no Listen item to attribute to, this falls back to highlighting | |
| // Home as a default categorisation only, so it shouldn't be announced. | |
| if ( | |
| pageType === AUDIO_PAGE || | |
| pageType === LIVE_RADIO_PAGE || | |
| (pageType === MEDIA_ARTICLE_PAGE && primaryMediaType === 'audio') || | |
| (pageType === ARTICLE_PAGE && primaryMediaType === 'audio') | |
| ) { | |
| return listenIndex > -1 | |
| ? { index: listenIndex, shouldAnnounce: true } | |
| : { index: 0, shouldAnnounce: false }; | |
| } | |
| // Video pages, video mediaArticles, and article pages with a video primaryMediaType. | |
| // Audio pages, audio mediaArticles, and article pages with an audio primaryMediaType | |
| // If there's no Watch or Listen item to attribute to, this falls back to highlighting | |
| // Home as a default categorisation only, so it shouldn't be announced. | |
| if ( | |
| pageType === TV_PAGE || | |
| pageType === LIVE_TV_PAGE || | |
| pageType === AUDIO_PAGE || | |
| pageType === LIVE_RADIO_PAGE || | |
| (pageType === MEDIA_ARTICLE_PAGE && primaryMediaType === 'video') || | |
| (pageType === ARTICLE_PAGE && primaryMediaType === 'video') || | |
| (pageType === MEDIA_ARTICLE_PAGE && primaryMediaType === 'audio') || | |
| (pageType === ARTICLE_PAGE && primaryMediaType === 'audio') | |
| ) { | |
| return focusableItemIndex > -1 | |
| ? { index: focusableItemIndex, shouldAnnounce: true } | |
| : { index: 0, shouldAnnounce: false }; | |
| } |
…on' of github.com:bbc/simorgh into WS-2117-add-lightweight-contextual-navigation-attribution
https://bbc.atlassian.net/browse/WS-2117
I have updated this so that the home tab is highlighted when watch or listen is not highlighted, so it acts as a default or fallback. When the home page nav item is active it will only say 'current page' if the user is actually on the home page, otherwise it will just read 'home', link and list of 3, etc.
This pull request enhances the navigation highlighting logic to more accurately reflect the user's current page context, especially for media pages (audio/video). The main improvement is the introduction of page-type and media-type-based attribution for navigation highlighting, ensuring that "Watch" or "Listen" are highlighted when appropriate, rather than always defaulting to "Home" or having nothing highlighted. This is achieved through changes in the navigation logic by passing the detected primary media type through the component tree for article pages containing media.
Navigation logic improvements:
getActiveTopIndexinNavigation/index.tsxto use page type, detectedprimaryMediaType, and a new type field made available via the BFF to determine which top navigation item ("Home", "Watch", or "Listen") should be highlighted. This ensures that, for example, video pages highlight "Watch" and audio pages highlight "Listen", with sensible fallbacks when those items are not present. [1] [2]primaryMediaTypeis now detected inPageLayoutWrapper/index.tsxby inspectingblockTypesand content blocks, and is passed down throughHeaderContainertoNavigation. [1] [2] [3] [4]Test coverage improvements:
Navigation/index.test.tsxto verify correct navigation highlighting for a wide variety of page and media types, including articles, topics, TV, live TV, media articles, audio, and live radio pages, with and without corresponding navigation items.Type and prop updates:
PageLayoutWrapper,HeaderContainer, andNavigationcomponents to support the newprimaryMediaTypeprop.Navigationsupports the newtypeprop also. [1] [2] [3]Imports and constants:
These changes collectively ensure that navigation highlighting is context-aware, improving usability for users on media-rich pages.
It should work for radio pages, tv pages, and article and media article pages containing media.
Testing
Some example URLs
http://localhost:7081/afrique/articles/crevwjxjdgyo?renderer_env=live
http://localhost:7081/afrique/articles/cd9wdkqjgk9o?renderer_env=live
http://localhost:7081/afrique/bbc_afrique_tv/tv_programmes/w13xttmz?renderer_env=test
http://localhost:7081/afrique/bbc_afrique_radio/w1730k9s84l43h7?renderer_env=test
What we want to see is:
This branch is currently on preview 2 environment. The cache has temporarily been increased to 15 minutes to make testing easier because what you need to do to get the test iSite config through for the nav, and for this to work, is to
After the cache timeout time, you will need to revisit with ?renderer_env=test again.
Useful Links