Skip to content

Comments

Ws 2145 create navigation component#13713

Draft
LilyL0u wants to merge 95 commits intolatestfrom
ws-2145-create-navigation-component
Draft

Ws 2145 create navigation component#13713
LilyL0u wants to merge 95 commits intolatestfrom
ws-2145-create-navigation-component

Conversation

@LilyL0u
Copy link
Contributor

@LilyL0u LilyL0u commented Feb 12, 2026

Resolves JIRA:

Summary

A very high-level summary of easily-reproducible changes that can be understood by non-devs, and why these changes where made.

Code changes

  • List key code changes that have been made.

Testing

  1. List the steps required to test this PR.

Useful Links

@pvaliani
Copy link
Contributor

Shall we add a description/details to this now as its for review?

}

// Only fetch new nav for Arabic and Tamil services on Local/Test
if (service === 'arabic' && !isLive()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This only has arabic - should we be adding tamil to the check here as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

I can amend these checks to include Tamil. We'll need the backend change applied first to allow services other than Arabic to use the 'useNewNav' param, otherwise Tamil will try and render existing nav items in the new nav element.

Copy link
Contributor

Choose a reason for hiding this comment

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

I still haven't been able to sort the audit issue otherwise it would be deployed, apologies 😅 😬

Copy link
Contributor

Choose a reason for hiding this comment

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

No worries, I've made a small config so its a bit easier to rollout: 13685be

/**
* Prefer server-provided navItems; fallback to ServiceContext.navigation if missing.
*/
const navigationItems = navItems || navFromServiceConfig;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this technically crash if nav data is missing or undefined from isite? I think we'll always have the forms but just thinking navigationItems is used before we verify it exists, then we index into it directly

topActiveIndex > -1 ? navigationItems[topActiveIndex] : navigationItems[0];

The old nav had this null check

if (!navigation || navigation.length === 0) {

Copy link
Contributor

Choose a reason for hiding this comment

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

Technically it could, but the service configs should always have the fallback. I can add in the guard though for parity.

Copy link
Contributor

Choose a reason for hiding this comment

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

No worries, your call just wanted to point it out! I think that's fair if we always have the fallback

Copy link
Contributor

Choose a reason for hiding this comment

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

Added in 87bf622

Copy link
Contributor

@pvaliani pvaliani left a comment

Choose a reason for hiding this comment

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

Nice work @LilyL0u and thanks @amoore108 for continuing working on it. Be good to get it updated in the next iterations!

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