[MWPW-151517] - Remove condition for promobar hidden on mobile from gnav #2538

Deva309 · 2024-07-04T04:18:56Z

Description

Removed the condition that hides the gnav promobar on mobile. We now have the ability to set separate copies for each viewport. The acceptance criteria to on and off the promobar only for mobile comes with a CLS issue, so it is not part of this PR, as discussed with the PDMs.
For example:

To avoid CLS impact, we need to adhere to the character count limits specified in the design specs for the feds promo bar, also mentioned in the ticket. Additionally, this information has been included in the authoring documentation for the aside promo bar.

Resolves:

Test URLs:

codecov · 2024-07-04T04:21:30Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.70%. Comparing base (c0395ae) to head (a2d8d53).
Report is 4 commits behind head on stage.

Additional details and impacted files

@@           Coverage Diff           @@
##            stage    #2538   +/-   ##
=======================================
  Coverage   95.69%   95.70%           
=======================================
  Files         172      172           
  Lines       45359    45359           
=======================================
+ Hits        43406    43409    +3     
+ Misses       1953     1950    -3

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

libs/styles/styles.css

aem-code-sync · 2024-07-08T03:52:57Z

Page	Scores	Audits	Google
/drafts/devashish/promo-bar/feds-promobar?martech=off

mokimo · 2024-07-08T13:28:27Z

libs/styles/styles.css

@@ -4,7 +4,7 @@
  --global-height-nav: 64px;
  --global-height-breadcrumbs: 33px;
  /* stylelint-disable-next-line custom-property-pattern */
-  --global-height-navPromo: 65px;
+  --global-height-navPromo: 72px;


Have there been any considerations regarding the two-line, 80px use-case utilizing a min-height here? To me it seems like that use-case would lead to CLS currently. (Suggested CLS Strategy within the design specs)

According to the design specifications, the goal is to ensure that the feds promobar displays as a single line on all viewports. This character count limit is also included in the authoring documentation, following the design specifications.
So, with that we can ignore the case of two-line copy.

I noticed it spawns on two lines between 1200 and 1270px

@narcis-radu is right, the Promo spans two rows between 1200 and 1287px. Either the copy needs to be adapted or some styles.

Yes. Based on discussions with Joel Blytt the character count limit in design specs will be updated so that promobar displays on single line for this range also.

test/blocks/global-navigation/global-navigation.test.js

github-actions · 2024-07-09T01:18:06Z

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.

sharmrj · 2024-07-09T07:35:22Z

test/blocks/global-navigation/global-navigation.test.js

+      promoMeta.remove();
+    });
+
+    it('Exist on mobile', async () => {


I think we could have more descriptive test names that tell the reader when the promobar should or should not exist on mobile, i.e. when we have content for mobile

mokimo · 2024-07-09T10:41:13Z

Let's wait for the consonant/design sign-off on the ticket before merging this. From what I've seen, once you got a branch ready - you send that over on the Jira, have them sign off and then raise the PR. @ryanmparrish would that be the correct flow involving any (design) changes for blocks?

prativas22 · 2024-07-10T13:45:17Z

Validated the promobar functionality in Devices and observed that after enabling the promobar for the device, the Gnav does not show up on both the iPhone and the iPad. For Gnav, the iPhone does not display the Hamberg symbol.

Note: Android device is working as expected

URL:
https://mwpw-151517-promobar--milo--deva309.hlx.page/drafts/prativa/aside-promo-bar

iphone device: (both safari and chrome browser have same result)
Safari browser:

landscape orientation: no Gnav

iPad Device:
Safari

In iPad landscape orientation, the gnav displays as expected:

A JIRA ticket has been created to track the details. Refer to https://jira.corp.adobe.com/browse/MWPW-154090 for further details. Validation would be done after the bug fix.

@salonijain3 @sigadamvenkata

mokimo · 2024-07-12T06:26:52Z

@Deva309 there seems to be design feedback on the Ticket

… issue

…mobar

milo-pr-merge · 2024-07-18T17:07:54Z

Error merging 2538: [MWPW-151517] - Remove condition for promobar hidden on mobile from gnav Base branch was modified. Review and try the merge again.

milo-pr-merge · 2024-07-18T17:25:17Z

Error merging 2538: [MWPW-151517] - Remove condition for promobar hidden on mobile from gnav 1 review requesting changes and 3 approving reviews by reviewers with write access.

…mobar

narcis-radu · 2024-07-19T08:25:49Z

To avoid CLS impact, we need to adhere to the character count limits specified in the design specs for the feds promo bar, also mentioned in the ticket

@Deva309 - I see that between 1200px and 1270px the promo bar content is displayed on two rows

narcis-radu · 2024-07-19T08:32:27Z

libs/blocks/global-navigation/global-navigation.css

@@ -571,6 +570,11 @@ header.global-navigation {
    overflow: auto;
  }

+  .global-navigation.has-promo .feds-navItem--megaMenu .feds-popup {
+    max-height: calc(100vh - 100% - var(--global-height-navPromo));
+    box-sizing: border-box;


Most of the elements in the global navigation are context-box and this might generate some confusion when setting padding

@narcis-radu Yes, but this case involves the megamenu, where scrolling is necessary if its height and width exceed the screen dimensions. To address this, we calculate the height for the megamenu and add a scroll. However, due to content-box, it only sets the scroll for the content area, but we need scrolling for both the content and the surrounding spacing. I've now moved it closer to overflow:auto, which is only required for the scroll functionality.

We already have scrolling functionality on Production and the box-sizing is content-box, don't we ?

I think this was needed to compensate the 40px padding which was not included in height and the bottom-most link in the large menu was getting cut off from the scrolling.

libs/blocks/mobile-app-banner/mobile-app-banner.js

narcis-radu · 2024-07-19T08:35:47Z

libs/styles/styles.css

@@ -4,7 +4,7 @@
  --global-height-nav: 64px;
  --global-height-breadcrumbs: 33px;
  /* stylelint-disable-next-line custom-property-pattern */
-  --global-height-navPromo: 65px;
+  --global-height-navPromo: 72px;


I noticed it spawns on two lines between 1200 and 1270px

prativas22 · 2024-07-19T11:05:13Z

The promobar functionality is verified after the resizing and scrolling issue was fixed.
URL:
https://mwpw-151517-promobar--milo--deva309.hlx.page/drafts/devashish/promo-bar/feds-promobar

https://mwpw-151517-promobar--milo--deva309.hlx.page/drafts/prativa/feds-promobar-one
https://mwpw-151517-promobar--milo--deva309.hlx.page/drafts/prativa/aside-promo-bar

Scrolling

without promobar:

Deva309 · 2024-07-19T11:20:03Z

To avoid CLS impact, we need to adhere to the character count limits specified in the design specs for the feds promo bar, also mentioned in the ticket

@Deva309 - I see that between 1200px and 1270px the promo bar content is displayed on two rows

@narcis-radu
Yes, I missed this range. I will check with the PMs to see if the range of 1200px to 1286px, where the content for the feds promobar wraps to two lines, applies to any widely used devices. If it does, we will need to update the character count limit.

overmyheadandbody

The flex-wrap declaration should be revisited, as it has the potential to affect a large number of production pages.

overmyheadandbody · 2024-07-23T13:40:20Z

libs/blocks/global-navigation/global-navigation.css

@@ -702,5 +706,6 @@ header.global-navigation {

  .feds-navItem--megaMenu .feds-popup {
    align-items: center;
+    flex-wrap: wrap;


Why is this needed? Is it desirable to have columns span on multiple rows in certain scenarios? Doesn't this interfere with the feds-crossCloudMenu-wrapper element, since it's also inside the popup? This is how this tweak affects the current Photoshop page:

Since this doesn't seem to be related to the promo, it might be good to remove this.

This was added to fix one of the issues where the left side of the large menu gets cut from left side (1200-1300px range). I'll remove this due to above issue and as its unrelated to promobar change. We will log a separate bug for this.

5 columns have never been supported in the dropdown. The initial Consonant guidelines (which I can't locate currently due to how many changes they had on their platform 😅 ) stated that a maximum of 4 columns should be added in a dropdown menu.

overmyheadandbody · 2024-07-23T13:44:24Z

libs/styles/styles.css

@@ -4,7 +4,7 @@
  --global-height-nav: 64px;
  --global-height-breadcrumbs: 33px;
  /* stylelint-disable-next-line custom-property-pattern */
-  --global-height-navPromo: 65px;
+  --global-height-navPromo: 72px;


@narcis-radu is right, the Promo spans two rows between 1200 and 1287px. Either the copy needs to be adapted or some styles.

overmyheadandbody · 2024-07-23T13:48:35Z

libs/blocks/global-navigation/global-navigation.css

@@ -569,6 +568,11 @@ header.global-navigation {
    padding: 40px 0 0;
    max-height: calc(100vh - 100%);
    overflow: auto;
+    box-sizing: border-box;


I don't see any box-size model properties adapted to justify this change. Why is it needed?

I think this was needed to compensate the 40px padding which was not included in height and the bottom-most link in the large menu was getting cut off from the scrolling as shown.

libs/styles/styles.css

overmyheadandbody

Thanks for making the changes!

prativas22 · 2024-07-26T04:44:30Z

Validation done for the promobar
https://mwpw-151517-promobar--milo--deva309.hlx.page/drafts/prativa/aside-promo-bar
https://mwpw-151517-promobar--milo--deva309.hlx.page/drafts/prativa/test-feds-promobar-with-app-banner
https://mwpw-151517-promobar--milo--deva309.hlx.page/drafts/prativa/feds-promobar-char-limit
https://mwpw-151517-promobar--milo--deva309.hlx.page/drafts/prativa/test-feds-promobar
iphone:

samsung:

Resizing screen

Localization page:
https://mwpw-151517-promobar--milo--deva309.hlx.page/fr/drafts/prativa/aside-promo-bar

https://mwpw-151517-promobar--milo--deva309.hlx.page/ae_ar/drafts/prativa/aside-promo-bar

* stage: MWPW-150557 - Split Marquee CLS issues on consuming sites (adobecom#2636) Mwpw 147034: Custom border color + badge/border color decoupling [merch card] (adobecom#2613) [MWPW-151517] - Remove condition for promobar hidden on mobile from gnav (adobecom#2538) # Conflicts: # libs/deps/merch-card.js

* stage: [MWPW-153611] [Gray Box] environment aware links (adobecom#2622) MWPW-153580: Add Opt-In Feature for CaaS Badge Display (adobecom#2625) [MWPW-154335] [callout] Spacing issue encountered when the call-out section is added (adobecom#2628) MWPW-150557 - Split Marquee CLS issues on consuming sites (adobecom#2636) Mwpw 147034: Custom border color + badge/border color decoupling [merch card] (adobecom#2613) [MWPW-151517] - Remove condition for promobar hidden on mobile from gnav (adobecom#2538) MWPW-154998 [MEP][MILO] Manifests do not execute in the right order when there is a disabled manifest (adobecom#2632) mwpw-154965: Fetch federal stage content from hlx.page instead of stage.adobe.com (adobecom#2618) Correct error messages for duplicate files on the stage to main workflow (adobecom#2621) MWPW-153245 [merch][analytics] dispatch wcomp events, and let default lh (adobecom#2610) Revert "MWPW-146528[MILO][MEP][ANALYTICS] Add attribute to content changed by Target for analytics and MWPW-152274" (adobecom#2627) MWPW-128600 Locale Tool: Langstore points to langstore/en (adobecom#2615) Fix for errors in dynamically loaded scripts in test cases (adobecom#2619) MWPW-146528[MILO][MEP][ANALYTICS] Add attribute to content changed by Target for analytics and MWPW-152274 (adobecom#2593) Bootstrapper script for milo feds blocks (adobecom#2560) Revert "[MWPW-152968] mWeb - Passing ECID to Branch.io banner - Implementation" (adobecom#2612) # Conflicts: # libs/deps/merch-card.js

Deva309 added 2 commits July 3, 2024 10:24

Remove condition for promobar hidden on mobile from gnav

dcea240

Fix lint and UT

429001e

Deva309 requested review from salonijain3, bandana147, sharmrj, nishantka and sonawanesnehal3 as code owners July 4, 2024 04:18

overmyheadandbody requested changes Jul 4, 2024

View reviewed changes

libs/styles/styles.css Outdated Show resolved Hide resolved

Adjust height for feds promobar

9e0a3ef

brnach banner not shown in case feds promobar is present

85727af

Deva309 requested a review from overmyheadandbody July 8, 2024 12:46

mokimo reviewed Jul 8, 2024

View reviewed changes

test/blocks/global-navigation/global-navigation.test.js Outdated Show resolved Hide resolved

Deva309 added 2 commits July 9, 2024 10:30

Add UT for promobar hide/show on mobile

c1ccdaa

Lint Fix

abcc080

Deva309 requested a review from mokimo July 9, 2024 05:26

sharmrj approved these changes Jul 9, 2024

View reviewed changes

bandana147 approved these changes Jul 9, 2024

View reviewed changes

spadmasa assigned prativas22 Jul 10, 2024

spadmasa added verified PR has been E2E tested by a reviewer and removed verified PR has been E2E tested by a reviewer labels Jul 10, 2024

spadmasa added the do not merge PR should not be merged yet label Jul 10, 2024

Deva309 added 2 commits July 16, 2024 10:18

Removed condition for on and off feds promobar on mobile to avoid CLS…

07638b9

… issue

Merge branch 'stage' of github.com:adobecom/milo into MWPW-151517-pro…

b2debae

…mobar

Merge branch 'stage' of github.com:adobecom/milo into MWPW-151517-pro…

3570a90

…mobar

narcis-radu reviewed Jul 19, 2024

View reviewed changes

spadmasa added needs-verification PR requires E2E testing by a reviewer and removed verified PR has been E2E tested by a reviewer Ready for Stage labels Jul 19, 2024

Deva309 added 3 commits July 19, 2024 15:06

Fix review comments

d28cea4

Fix megamenu scroll issue

4655b21

reverted condition

a91f411

Deva309 requested a review from narcis-radu July 19, 2024 12:02

overmyheadandbody requested changes Jul 23, 2024

View reviewed changes

remove flex wrap

58ba4ae

overmyheadandbody reviewed Jul 24, 2024

View reviewed changes

libs/styles/styles.css Show resolved Hide resolved

mokimo self-requested a review July 24, 2024 16:54

nishantka added 2 commits July 25, 2024 12:33

Merge branch 'stage' into MWPW-151517-promobar

33992d6

add initial visibility hidden for gnav

a2d8d53

nishantka requested a review from overmyheadandbody July 25, 2024 07:49

overmyheadandbody approved these changes Jul 25, 2024

View reviewed changes

spadmasa 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 Jul 26, 2024

milo-pr-merge bot merged commit 386f817 into adobecom:stage Jul 29, 2024
21 checks passed

milo-pr-merge bot mentioned this pull request Jul 29, 2024

[Release] Stage to Main #2641

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MWPW-151517] - Remove condition for promobar hidden on mobile from gnav #2538

[MWPW-151517] - Remove condition for promobar hidden on mobile from gnav #2538

Deva309 commented Jul 4, 2024 •

edited by nishantka

Loading

codecov bot commented Jul 4, 2024 •

edited

Loading

aem-code-sync bot commented Jul 8, 2024 •

edited

Loading

mokimo Jul 8, 2024 •

edited

Loading

Deva309 Jul 9, 2024

narcis-radu Jul 19, 2024

overmyheadandbody Jul 23, 2024

nishantka Jul 24, 2024

github-actions bot commented Jul 9, 2024

sharmrj Jul 9, 2024

mokimo commented Jul 9, 2024

prativas22 commented Jul 10, 2024

mokimo commented Jul 12, 2024

milo-pr-merge bot commented Jul 18, 2024

milo-pr-merge bot commented Jul 18, 2024

narcis-radu commented Jul 19, 2024

narcis-radu Jul 19, 2024

Deva309 Jul 19, 2024

narcis-radu Jul 19, 2024

nishantka Jul 24, 2024

narcis-radu Jul 19, 2024

prativas22 commented Jul 19, 2024

Deva309 commented Jul 19, 2024

overmyheadandbody left a comment

overmyheadandbody Jul 23, 2024

nishantka Jul 24, 2024

overmyheadandbody Jul 24, 2024

overmyheadandbody Jul 23, 2024

overmyheadandbody Jul 23, 2024

nishantka Jul 24, 2024

overmyheadandbody left a comment

prativas22 commented Jul 26, 2024 •

edited

Loading

[MWPW-151517] - Remove condition for promobar hidden on mobile from gnav #2538

[MWPW-151517] - Remove condition for promobar hidden on mobile from gnav #2538

Conversation

Deva309 commented Jul 4, 2024 • edited by nishantka Loading

Description

codecov bot commented Jul 4, 2024 • edited Loading

Codecov Report

aem-code-sync bot commented Jul 8, 2024 • edited Loading

mokimo Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 9, 2024

Choose a reason for hiding this comment

mokimo commented Jul 9, 2024

prativas22 commented Jul 10, 2024

mokimo commented Jul 12, 2024

milo-pr-merge bot commented Jul 18, 2024

milo-pr-merge bot commented Jul 18, 2024

narcis-radu commented Jul 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prativas22 commented Jul 19, 2024

Deva309 commented Jul 19, 2024

overmyheadandbody left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

overmyheadandbody left a comment

Choose a reason for hiding this comment

prativas22 commented Jul 26, 2024 • edited Loading

Deva309 commented Jul 4, 2024 •

edited by nishantka

Loading

codecov bot commented Jul 4, 2024 •

edited

Loading

aem-code-sync bot commented Jul 8, 2024 •

edited

Loading

mokimo Jul 8, 2024 •

edited

Loading

prativas22 commented Jul 26, 2024 •

edited

Loading