[MWPW-158932] - Add support for custom links through link group in parallel to large menu. #2942
Conversation
Deva309
requested review from
salonijain3,
bandana147,
sharmrj,
nishantka and
sonawanesnehal3
as code owners
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## stage #2942 +/- ##
==========================================
- Coverage 96.30% 95.97% -0.34%
==========================================
Files 242 175 -67
Lines 54873 46888 -7985
==========================================
- Hits 52845 45000 -7845
+ Misses 2028 1888 -140 ☔ View full report in Codecov by Sentry. |
|
Reminder to set the |
|
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. |
|
Validation done on the given url in desktop and devices Authoring done :
In devices detailed testing details are updated in substask of https://jira.corp.adobe.com/browse/MWPW-158932 |
|
Skipped merging 2942: [MWPW-158932] - Add support for custom links through link group in parallel to large menu. due to failing checks |
| @@ -696,7 +702,7 @@ class Gnav { | |||
| this.elements.mainNav.style.removeProperty('padding-bottom'); | |||
| } else { | |||
| const offset = Math.ceil(this.elements.topnavWrapper.getBoundingClientRect().bottom); | |||
| this.elements.mainNav.style.setProperty('padding-bottom', `${offset}px`); | |||
| this.elements.mainNav.style.setProperty('padding-bottom', `${2 * offset}px`); | |||
There was a problem hiding this comment.
Is this intentional? The purpose of this was to ensure content that would risk flowing outside of the page when the menu is too long remains accessible. This change, along with https://github.com/adobecom/milo/pull/2942/files#diff-af209f212893e015b3b74f7891e85e68f79801c7f98e760f8d56887deea3e4b3R64, make the experience and code more convoluted with no obvious benefit.
With the CSS changes, it might be that this piece of logic is no longer needed.
There was a problem hiding this comment.
This issue occurs specifically on iPhone devices when there are multiple links in that section and scrolling becomes necessary. I understand that a CSS fix would eliminate the need for extra spacing below. However, the fix we applied is for iPhones using Chrome and Safari browsers:
Case 1: When the URL is loaded and the menu is opened via the hamburger icon, some links become hidden due to the presence of back/forward arrows for navigation in the lower section. See the screenshot for reference:
Case 2: When the page is scrolled slightly, the bottom section disappears, allowing all the links to become visible.
To resolve Case 1, we increased the padding size. We can move this padding-bottom adjustment to CSS instead of using JavaScript, and remove that functionality from the global navigation.
| const mainNavItem = this.decorateMainNavItem(item, index); | ||
| if (mainNavItem) { | ||
| this.elements.mainNav.appendChild(mainNavItem); | ||
| } |
There was a problem hiding this comment.
What's the use case in which an element wouldn't be returned? Looking at
, an element is always returned. Most decoration methods should either return an element or'' (not null). So the methods should handle this, not the places where they're used.
|
Spoke with @mokimo that this was promised to be merged, but it still wouldn't hurt to have a look over the comments and integrate whatever is deemed valuable in another iteration. CC: @Deva309, @salonijain3 |
…rallel to large menu. (adobecom#2942) * Add support for custom links through link group in parallet to large menu * Added UT for the changes * lint fixes * Bug fix mwpw-159018 * Bug fix mwpw-159018 * Bug fix mwpw-159018
Adding support for adding custom links through
Link Groupalready a existing block used in GNAV and these custom links will be shown as per the config pass through standalone GNAV. These links are in parallel to thelarge-menuso authoring would be same like how we author large menus.Here is the screenshot how
Link Groupused in GNAV:Example: GNAV docx
Github Discussion for more details: https://github.com/orgs/adobecom/discussions/2901
Resolves: MWPW-158932
Issues:
MWPW-159018
Test URLs:
QA
https://adobecom.github.io/nav-consumer/navigation.html?env=stage&navbranch=mwpw-158932&authoringpath=/federal/dev/devashish&customlinks=home