Create a container element for search in standalone gnav #2833
Conversation
sonawanesnehal3
requested review from
salonijain3,
bandana147,
sharmrj,
nishantka and
Deva309
as code owners
|
Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
libs/navigation/navigation.js
Outdated
There was a problem hiding this comment.
Unexpected console statement.
milo/libs/navigation/navigation.js
Line 43 in b67ae71
There was a problem hiding this comment.
I don't remember the context for this one, but having console errors might not be desirable. Would LANA logs not be enough?
There was a problem hiding this comment.
This is for standalone Gnav in which we need to notify the consuming clients of the error.
|
sonawanesnehal3
added
the
needs-verification
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## stage #2833 +/- ##
==========================================
+ Coverage 95.89% 96.08% +0.19%
==========================================
Files 173 215 +42
Lines 46126 53947 +7821
==========================================
+ Hits 44231 51835 +7604
- Misses 1895 2112 +217 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Had a small chat with @narcis-radu regarding this, seems like is the least evil option. Looks good in that case!
| this.elements.customSearch = toFragment`<div class="feds-client-search"></div>`; | ||
| return this.elements.customSearch; |
There was a problem hiding this comment.
It doesn't look like you're using this.elements.customSearch anywhere, so why not just
| this.elements.customSearch = toFragment`<div class="feds-client-search"></div>`; | |
| return this.elements.customSearch; | |
| return toFragment`<div class="feds-client-search"></div>`; |
Alternatively, you could have the searchEnabled check here and return the element or '' and then just call the method directly in decorateMainNav.
There was a problem hiding this comment.
Hi @overmyheadandbody ,
Updated the code. Please review again.
libs/navigation/navigation.js
Outdated
There was a problem hiding this comment.
I don't remember the context for this one, but having console errors might not be desirable. Would LANA logs not be enough?
|
This PR is currently in the |
|
Validations has ben done on the pr on the below url https://adobecom.github.io/nav-consumer/navigation.html?searchEnabled=on&navbranch=mwpw157417&env=stage |
spadmasa
added
verified
|
Reminder to set the |
| @@ -822,6 +822,7 @@ class Gnav { | |||
| ${isDesktop.matches ? '' : this.decorateSearch()} | |||
| ${this.elements.mainNav} | |||
| ${isDesktop.matches ? this.decorateSearch() : ''} | |||
| ${getConfig().searchEnabled === 'on' ? toFragment`<div class="feds-client-search"></div>` : ''} | |||
There was a problem hiding this comment.
One thing is not very clear to me. This feature is supposed to be relevant for the standalone Gnav, yet the config is added through scripts.js at a consumer level. Couldn't a config be passed directly to the standalone version? I feel like I'm missing something, but also want to make sure we're not polluting scripts.js with irrelevant properties.
There was a problem hiding this comment.
@overmyheadandbody There is no separate config for standalone Gnav. The standalone version directly calls the init of global-navigation /global-footer and we set the miloLibs from navigation.js not through script.js.
Create a container element for search in standalone gnav
Resolves: MWPW-157417
Test URLs:
QA