MWPW-145574 - Dynamic Nav Gnav Implementation

[JasonHowellSlavin]

Adds functionality to allow consumer projects to opt in to a dynamic nav experience. If a dynamicNavKey is registered in a consumer repo's config, this code adds logic to save the gnav source to sessionStorage allowing for the same nav experience in subsequent clicks.

• entry pages will set gnavSource to sessionStorage is a gnav source meta tag is present
• on pages will check for a gnavSource in sessionStorage, and if one is found, either modify the gnav-source meta tag, or create one
• pages without the dynamic-nav metadata will do nothing or remove gnavSource from sessionStorage
• projects without the key present, or where the key mismatches with the key saved in storage will use the default gnav experience

Context:
The wiki in the ticket provides more context, but it was desired by our stakeholders that this only be a single tab experience.

Based on comments in #2092 I am adding this PR as the approach and closing 2092.

Resolves: MWPW-145574

Test URLs:

Before: https://main--milo--adobecom.hlx.page/drafts/slavin/dynamic-nav/hugo-boss-case-study-entry?martech=off
After: https://dynamic-nav-gnav-impl--milo--jasonhowellslavin.hlx.page/drafts/slavin/dynamic-nav/hugo-boss-case-study-entry?martech=off
Before: https://main--bacom--adobecom.hlx.page/drafts/slavin/dynamic-nav/aem-rtcdp-entry?martech=off
After: https://dynamic-nav-gnav-impl-config--bacom--adobecom.hlx.page/drafts/slavin/dynamic-nav/aem-rtcdp-entry?milolibs=dynamic-nav&martech=off

To test:
Go to the second after link. On the Hugo Boss page
Observe the nav (pricing and resources)
Right click "watch now" and open in a new tab
Observe the nav is the default milo nav ("Explore")
From the Hugo Boss page left click "Watch Now" to open in the same tab
Observe the pricing and resources nav loads

[aem-code-sync]

Page	Scores	Audits	Google
/drafts/slavin/dynamic-nav/hugo-boss-case-study-entry?martech=off

[sharmrj]

I think we should probably have a new function getGnavSource (or something along those lines) that produces the url and contains all the logic for generating the url within it.

[bandana147]

I agree with Raghav, we should extract it into a separate function that generates the URL.

[JasonHowellSlavin]

Hmm. I am not so sure that I agree. The dynamic nav is an opt in feature requiring a consumer to update their config to have the dynamicNavKey in order to kick off the experience.

Though it may make the init function a bit cleaner, a function combining the two would mix both the default behavior and an opt in feature, which I am not certain aids in readability.

Interested to see what you think.

[sharmrj]

In the future we may have logic added or subtracted from our hypothetical getSource function. init is and should be agnostic about the details of getting the url and that is communicated well by a separate function.

The separation between default and opt in behaviour can be communicated well enough in the function itself by the if statement you'll have to add. And the notion of what constitutes default vs non default behavior now and in the future is malleable enough that it makes sense to cordon it off from our main init logic so that it's clear in the future where gnav url generation logic should go.

[JasonHowellSlavin]

I appreciate your thoughtful and full response! I see what you're saying. To be honest, I am still on the fence because it feels a bit like premature optimization and we'd be adding 3 lines of code by pulling these 5 out into a function.

Maybe @mokimo can weigh in since he recommended the approach.

[mokimo]

@sharmrj So you'd think something like this?

const getSource = async () => {
    const { locale, dynamicNavKey } = getConfig();
    let url = getMetadata('gnav-source') || `${locale.contentRoot}/gnav`;
    if (dynamicNavKey) {
      const { default: dynamicNav } = await import('../../features/dynamic-navigation.js');
      url = dynamicNav(url, dynamicNavKey);
    }
    return url
}

export default async function init(block) {
  try {
    const { locale, mep } = getConfig();
    const url = await getSource()
    const content = await fetchAndProcessPlainHtml({ url })
      .catch((e) => lanaLog({
        message: `Error fetching gnav content url: ${url}`,
        e,
        tags: 'errorType=error,module=gnav',
      }));
....

@sharmrj and @bandana147 are the new gnav code owners 😁 So if that's how they'd prefer it, no objections.

Since the init function is growing already, it does look pretty clean indeed, extracting it into a small helper

[sharmrj]

Yup. That's what I was envisioning.

[JasonHowellSlavin]

Sounds good. I'll make those changes you requested @sharmrj. Any thoughts on modifying the unit tests? Since the function is called in init seems that the tests can remain as is.

I'll be sure to update the docs as well @mokimo.

Thank you both for your input!

[github-actions]

This pull request is not passing all required checks. Please see this discussion for information on how to get all checks passing. Inconsistent checks can be manually retried. If a test absolutely can not pass for a good reason, please add a comment with an explanation to the PR.

[mokimo]

I like the overall solution. It's an opt-in feature and only affects pages that actually use the feature.

It would be great if this can be added to the docs https://milo.adobe.com/docs/authoring/global-navigation

[github-actions]

This PR is currently in the needs-verification state. Please assign a QA engineer to verify the changes.

[github-actions]

This PR has not been updated recently and will be closed in 7 days if no action is taken. Please ensure all checks are passing, https://github.com/orgs/adobecom/discussions/997 provides instructions. If the PR is ready to be merged, please mark it with the "Ready for Stage" label.

[mokimo]

@JasonHowellSlavin what's the timing on this? Make sure to assign a QA/QE and have them apply the ready for stage label to take this forward

[github-actions]

Reminder to set the Ready for Stage label - to queue this to get merged to stage & production.