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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 18 commits into from
Jul 29, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
dcea240
Remove condition for promobar hidden on mobile from gnav
Deva309 Jul 3, 2024
429001e
Fix lint and UT
Deva309 Jul 4, 2024
9e0a3ef
Adjust height for feds promobar
Deva309 Jul 8, 2024
85727af
brnach banner not shown in case feds promobar is present
Deva309 Jul 8, 2024
c1ccdaa
Add UT for promobar hide/show on mobile
Deva309 Jul 9, 2024
abcc080
Lint Fix
Deva309 Jul 9, 2024
07638b9
Removed condition for on and off feds promobar on mobile to avoid CLS…
Deva309 Jul 16, 2024
b2debae
Merge branch 'stage' of github.com:adobecom/milo into MWPW-151517-pro…
Deva309 Jul 16, 2024
de2769d
Remove UT changes not needed now
Deva309 Jul 16, 2024
300b96e
Merge branch 'stage' of github.com:adobecom/milo into MWPW-151517-pro…
Deva309 Jul 18, 2024
01db884
fixed mega menu height if promobar is present
Deva309 Jul 18, 2024
3570a90
Merge branch 'stage' of github.com:adobecom/milo into MWPW-151517-pro…
Deva309 Jul 19, 2024
d28cea4
Fix review comments
Deva309 Jul 19, 2024
4655b21
Fix megamenu scroll issue
Deva309 Jul 19, 2024
a91f411
reverted condition
Deva309 Jul 19, 2024
58ba4ae
remove flex wrap
nishantka Jul 24, 2024
33992d6
Merge branch 'stage' into MWPW-151517-promobar
nishantka Jul 25, 2024
a2d8d53
add initial visibility hidden for gnav
nishantka Jul 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion libs/blocks/global-navigation/global-navigation.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,6 @@ header.global-navigation {

/* Promo */
.global-navigation .aside.promobar {
display: none; /* For when someone switches from desktop to mobile */
z-index: 1;
}

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Screenshot 2024-07-24 at 1 29 19 PM

}

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

[dir = "rtl"] .feds-navItem--megaMenu .feds-popup {
Expand Down
2 changes: 1 addition & 1 deletion libs/blocks/global-navigation/global-navigation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -777,7 +777,7 @@ class Gnav {
decorateAside = async () => {
this.elements.aside = '';
const promoPath = getMetadata('gnav-promo-source');
if (!isDesktop.matches || !promoPath) {
if (!promoPath) {
this.block.classList.remove('has-promo');
return this.elements.aside;
}
Expand Down
2 changes: 1 addition & 1 deletion libs/blocks/mobile-app-banner/mobile-app-banner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ function branchInit(key) {
/* eslint-enable */
export default async function init(el) {
const header = document.querySelector('.global-navigation');
if (!header) return;
if (!header || header.classList.contains('has-promo')) return;
narcis-radu marked this conversation as resolved.
Show resolved Hide resolved
const classListArray = Array.from(el.classList);
const product = classListArray.find((token) => token.startsWith('product-')).split('-')[1];
const key = await getKey(product);
Expand Down
13 changes: 6 additions & 7 deletions libs/styles/styles.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

@mokimo mokimo Jul 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

--feds-totalheight-nav: calc(var(--feds-height-nav, --global-height-nav) + var(--feds-height-breadcrumbs, --global-height-breadcrumbs));

/* Colors */
Expand Down Expand Up @@ -708,13 +708,12 @@ header.global-navigation {
visibility: hidden;
overmyheadandbody marked this conversation as resolved.
Show resolved Hide resolved
}

@media (min-width: 900px) {
header.global-navigation.has-promo {
height: auto;
/* stylelint-disable-next-line custom-property-pattern */
min-height: calc(var(--global-height-nav) + var(--global-height-navPromo));
}
header.global-navigation.has-promo {
height: auto;
min-height: calc(var(--global-height-nav) + var(--global-height-navPromo));
}

@media (min-width: 900px) {
header.global-navigation.has-breadcrumbs {
padding-bottom: var(--global-height-breadcrumbs);
}
Expand Down
9 changes: 0 additions & 9 deletions test/blocks/global-navigation/gnav-promo.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,13 +47,4 @@ describe('Promo', () => {
});
promoMeta.remove();
});

it('doesn\'t exist on mobile', async () => {
const promoMeta = toFragment`<meta name="gnav-promo-source" content="http://localhost:2000/fragments/correct-promo-fragment">`;
document.head.append(promoMeta);
const nav = await createFullGlobalNavigation({ viewport: 'mobile', hasPromo: true });
expect(nav.block.classList.contains('has-promo')).to.be.false;
const asideElem = nav.block.querySelector('.aside.promobar');
expect(asideElem).to.not.exist;
});
});