Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MWPW-157965: Upgrade Universal Nav to Version 1.3 #2867

Merged
merged 4 commits into from
Sep 17, 2024
Merged

MWPW-157965: Upgrade Universal Nav to Version 1.3 #2867

merged 4 commits into from
Sep 17, 2024

Conversation

nishantka
Copy link
Contributor

@nishantka nishantka commented Sep 11, 2024

Upgrades the UNav to 1.3

Adding eventListener for reloading Unav once Unav promise resolved as with v1.3 - the window.UniversalNav() returns a promise post which we can use methods like window.UniversalNav().reload() - ref
Also, Unav suggested to not rely on onReady method for triggering PEP modal instead we can wait for promise returned by window.UniversalNav() to be resolved.

Resolves: MWPW-157965

Test URLs:

Copy link
Contributor

aem-code-sync bot commented Sep 11, 2024

Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
In case there are problems, just click a checkbox below to rerun the respective action.

  • Re-run PSI checks
  • Re-sync branch
Commits

Copy link
Contributor

aem-code-sync bot commented Sep 11, 2024

Copy link

codecov bot commented Sep 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.18%. Comparing base (3fd877c) to head (9898620).
Report is 17 commits behind head on stage.

Additional details and impacted files
@@            Coverage Diff             @@
##            stage    #2867      +/-   ##
==========================================
+ Coverage   96.09%   96.18%   +0.08%     
==========================================
  Files         215      215              
  Lines       53954    53989      +35     
==========================================
+ Hits        51849    51929      +80     
+ Misses       2105     2060      -45     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nishantka nishantka added run-nala Run Nala Test Automation against PR needs-verification PR requires E2E testing by a reviewer labels Sep 11, 2024
@@ -608,9 +608,6 @@ class Gnav {
locale,
imsClientId: window.adobeid?.client_id,
theme: isDarkMode() ? 'dark' : 'light',
onReady: () => {
this.decorateAppPrompt({ getAnchorState: () => window.UniversalNav.getComponent?.('app-switcher') });
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Is onReady callback removed in new version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As mentioned in PR description - Unav suggested to not rely on onReady

Copy link
Contributor

@sharmrj sharmrj Sep 11, 2024

Choose a reason for hiding this comment

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

and this is true for 1.2 as well? (should someone want to use that version)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i don't think it affects 1.2 as the window.UNav() promise came up with 1.3

Copy link
Contributor

@sharmrj sharmrj left a comment

Choose a reason for hiding this comment

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

LGTM.

@@ -608,9 +608,6 @@ class Gnav {
locale,
imsClientId: window.adobeid?.client_id,
theme: isDarkMode() ? 'dark' : 'light',
onReady: () => {
this.decorateAppPrompt({ getAnchorState: () => window.UniversalNav.getComponent?.('app-switcher') });
},
Copy link
Contributor

@sharmrj sharmrj Sep 11, 2024

Choose a reason for hiding this comment

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

and this is true for 1.2 as well? (should someone want to use that version)

window.UniversalNav.reload(CONFIG.universalNav.universalNavConfig);
});
window.UniversalNav(CONFIG.universalNav.universalNavConfig)
.then(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Super minor, but I wonder if this won't be clearer with an await instead of a .then. That way we we get less nesting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure. I've updated this to await

Copy link
Contributor

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

@sigadamvenkata
Copy link

Verified the Unav 1.3 files and libs are loading in the given branch URL. Complete regression will be done across milo locales pages once PR merge to STAGE.

{2552412C-6228-4C5E-91A3-DB2C977F091F}

@sigadamvenkata sigadamvenkata added verified PR has been E2E tested by a reviewer Ready for Stage and removed needs-verification PR requires E2E testing by a reviewer labels Sep 17, 2024
@milo-pr-merge milo-pr-merge bot merged commit d51b989 into stage Sep 17, 2024
22 checks passed
@milo-pr-merge milo-pr-merge bot deleted the feds branch September 17, 2024 09:15
@milo-pr-merge milo-pr-merge bot mentioned this pull request Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready for Stage run-nala Run Nala Test Automation against PR verified PR has been E2E tested by a reviewer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants