SubdomainNavBar overflow fix (#451)

* wip * fix menu overflow bug * add MobileMenuOpenManyItems story * add changeset * remove commented out CSS * use windowSize hook to determine if menu should be hidden * github-actions[bot] Regenerated snapshots * don't show menu wrapper when menu is hidden * github-actions[bot] Regenerated snapshots * increase timeout for screenshot * github-actions[bot] Regenerated snapshots * github-actions[bot] Regenerated snapshots * reset snapshots * reset other snapshots * github-actions[bot] Regenerated snapshots * change MobileMenuOpenManyItems.parameters to iphone5 * github-actions[bot] Regenerated snapshots * increase timeout for Mobile Menu Open Many Items spec * github-actions[bot] Regenerated snapshots * increase timeout * github-actions[bot] Regenerated snapshots * github-actions[bot] Regenerated snapshots * CTAs should appear at exactly 768 pixels, not 769 * hide title unless the viewport is 'small' or greater * Update .changeset/friendly-deers-obey.md Co-authored-by: Rez <[email protected]> * use rem instead of px * only fix on < medium screens * github-actions[bot] Regenerated snapshots * reset snapshots to main * change rule to **max**-width * github-actions[bot] Regenerated snapshots * reset avatar snapshot * reset to 48rem * more reversions * revert * fix reverts * wip * fix * reset conflicts * github-actions[bot] Regenerated snapshots * reset avatar snapshot * revert SubdomainNavBar-button-area to inline-flex * hide titles when width is less than 'Small' * github-actions[bot] Regenerated snapshots * reset erroneous snapshots --------- Co-authored-by: joseph-lozano <[email protected]> Co-authored-by: Rez <[email protected]>
primer · Oct 24, 2023 · b15ab24 · b15ab24
1 parent 1896326
commit b15ab24
Show file tree

Hide file tree

Showing 12 changed files with 151 additions and 52 deletions.
diff --git a/.changeset/friendly-deers-obey.md b/.changeset/friendly-deers-obey.md
@@ -0,0 +1,7 @@
+---
+'@primer/react-brand': patch
+---
+
+Fixed menu item overflow issues on narrow viewports in `SubdomainNavBar`. 
+
+In addition, the `body` element will now hide overflow content and prevent background scrolling when the menu is open.
diff --git a/packages/e2e/scripts/playwright/playwright.generate-tests.ts b/packages/e2e/scripts/playwright/playwright.generate-tests.ts
@@ -41,6 +41,7 @@
     'components-subdomainnavbar--overflow-menu-open': 7500, // for the animation
     'components-subdomainnavbar--mobile-view': 5500, // for the animation
     'components-subdomainnavbar--mobile-menu-open': 5500, // for all staggered animations
+    'components-subdomainnavbar--mobile-menu-open-many-items': 5500, // for all staggered animations
     'components-subdomainnavbar--mobile-search-results-visible': 5500, // for the animation
     'components-subdomainnavbar--mobile-no-links': 5500, // for the animation
     'components-button-features--primary-focus-non-standard-bg': 2000, // for the interaction test

diff --git a/packages/react/src/SubdomainNavBar/NavigationVisbilityObserver.tsx b/packages/react/src/SubdomainNavBar/NavigationVisbilityObserver.tsx
@@ -14,7 +14,7 @@ import {useWindowSize} from '../hooks/useWindowSize'
 export function NavigationVisbilityObserver({children, className, ...rest}) {
   const navRef = useRef<HTMLUListElement | null>(null)
   const [visibilityMap] = useVisibilityObserver(navRef, children)
-  const {width} = useWindowSize()
+  const {isMedium} = useWindowSize()
 
   const showOverflow = Object.values(visibilityMap).includes(false)
 
@@ -24,12 +24,10 @@ export function NavigationVisbilityObserver({children, className, ...rest}) {
         return React.cloneElement(child, {
           className: clsx(
             child.props.className,
-            width &&
-              width >= 768 &&
+            isMedium &&
               !!visibilityMap[child.props['data-navitemid']] &&
               styles['SubdomainNavBar-primary-nav-list-item--visible'],
-            width &&
-              width >= 768 &&
+            isMedium &&
               !visibilityMap[child.props['data-navitemid']] &&
               styles['SubdomainNavBar-primary-nav-list-item--invisible'],
           ),

diff --git a/packages/react/src/SubdomainNavBar/SubdomainNavBar.module.css b/packages/react/src/SubdomainNavBar/SubdomainNavBar.module.css
@@ -140,17 +140,29 @@
     display: block;
     margin: 0;
     padding: 0;
+  }
+
+  .SubdomainNavBar-menu-wrapper {
+    display: flex;
+    flex-direction: column;
+    justify-content: space-between;
+    overflow-y: auto;
     position: absolute;
     top: 75px;
     left: 0;
     right: 0;
+    bottom: 0;
     z-index: 2;
     background-color: var(--brand-color-canvas-default);
     width: 100vw;
     height: calc(100vh - 75px);
     animation: fade-in-down 500ms;
     animation-timing-function: var(--brand-animation-easing-default);
   }
+
+  .SubdomainNavBar-menu-wrapper--close {
+    display: none;
+  }
 }
 
 @media screen and (min-width: 48rem) {
@@ -471,11 +483,9 @@
 
   .SubdomainNavBar-button-area--visible {
     display: flex;
-    position: absolute;
     left: 0;
     z-index: 99;
     width: 100%;
-    bottom: calc(calc(-100vh - 75px) - -150px);
     height: auto;
     animation-name: fade-in-down-staggered;
     animation-duration: 500ms;

diff --git a/packages/react/src/SubdomainNavBar/SubdomainNavBar.module.css.d.ts b/packages/react/src/SubdomainNavBar/SubdomainNavBar.module.css.d.ts
@@ -15,7 +15,9 @@ declare const styles: {
   readonly "SubdomainNavBar-primary-nav-list": string;
   readonly "SubdomainNavBar-primary-nav-list--invisible": string;
   readonly "SubdomainNavBar-primary-nav-list--visible": string;
+  readonly "SubdomainNavBar-menu-wrapper": string;
   readonly "fade-in-down": string;
+  readonly "SubdomainNavBar-menu-wrapper--close": string;
   readonly "SubdomainNavBar-primary-nav-list-item": string;
   readonly "fade-in-down-staggered": string;
   readonly "SubdomainNavBar-primary-nav-list-item--visible": string;

diff --git a/packages/react/src/SubdomainNavBar/SubdomainNavBar.stories.tsx b/packages/react/src/SubdomainNavBar/SubdomainNavBar.stories.tsx
@@ -405,17 +405,19 @@ const Template: StoryFn<Args> = args => {
     <>
       <div>
         <SubdomainNavBar {...args} title={args.title}>
-          {['collections', 'topics', 'articles', 'events', 'video', 'social'].slice(0, args.numLinks).map(link => {
-            return (
-              <SubdomainNavBar.Link key={link} href={`#${link}`}>
-                {link
-                  .toLowerCase()
-                  .split(' ')
-                  .map(word => word.charAt(0).toUpperCase() + word.slice(1))
-                  .join(' ')}
-              </SubdomainNavBar.Link>
-            )
-          })}
+          {['collections', 'topics', 'articles', 'events', 'video', 'social', 'podcasts', 'books', 'guides', 'webcasts']
+            .slice(0, args.numLinks)
+            .map(link => {
+              return (
+                <SubdomainNavBar.Link key={link} href={`#${link}`}>
+                  {link
+                    .toLowerCase()
+                    .split(' ')
+                    .map(word => word.charAt(0).toUpperCase() + word.slice(1))
+                    .join(' ')}
+                </SubdomainNavBar.Link>
+              )
+            })}
           {args.showSearch && (
             <SubdomainNavBar.Search
               ref={inputRef}
@@ -547,6 +549,18 @@ MobileMenuOpen.play = async ({canvasElement}) => {
   await userEvent.click(canvas.getByLabelText('Menu'))
 }
 
+export const MobileMenuOpenManyItems = Template.bind({})
+MobileMenuOpenManyItems.args = {numLinks: 10}
+MobileMenuOpenManyItems.parameters = {
+  viewport: {
+    defaultViewport: 'iphone5',
+  },
+}
+MobileMenuOpenManyItems.play = async ({canvasElement}) => {
+  const canvas = within(canvasElement)
+  await userEvent.click(canvas.getByLabelText('Menu'))
+}
+
 export const MobileSearchResultsVisible = Template.bind({})
 MobileSearchResultsVisible.parameters = {
   viewport: {

diff --git a/packages/react/src/SubdomainNavBar/SubdomainNavBar.tsx b/packages/react/src/SubdomainNavBar/SubdomainNavBar.tsx
@@ -7,6 +7,7 @@ import {NavigationVisbilityObserver} from './NavigationVisbilityObserver'
 import {useOnClickOutside} from '../hooks/useOnClickOutside'
 import {useFocusTrap} from '../hooks/useFocusTrap'
 import {useKeyboardEscape} from '../hooks/useKeyboardEscape'
+import {useWindowSize} from '../hooks/useWindowSize'
 
 /**
  * Design tokens
@@ -79,10 +80,22 @@ function Root({
 }: SubdomainNavBarProps) {
   const [menuHidden, setMenuHidden] = useState(true)
   const [searchVisible, setSearchVisible] = useState(false)
+  const {isSmall, isMedium} = useWindowSize()
 
   const handleMobileMenuClick = () => setMenuHidden(!menuHidden)
   const handleSearchVisibility = () => setSearchVisible(!searchVisible)
 
+  useEffect(() => {
+    if (isMedium) {
+      setMenuHidden(true)
+    }
+  }, [isMedium, menuHidden])
+
+  useEffect(() => {
+    const newOverflowState = menuHidden ? 'auto' : 'hidden'
+    document.body.style.overflow = newOverflowState
+  }, [menuHidden])
+
   const hasLinks =
     useMemo(
       () =>
@@ -141,7 +154,7 @@ function Root({
                   <MarkGithubIcon fill="currentColor" size={24} />
                 </a>
               </li>
-              {title && (
+              {title && isSmall && (
                 <>
                   <li role="separator" className={styles['SubdomainNavBar-title-separator']} aria-hidden>
                     /
@@ -204,42 +217,83 @@ function Root({
               </button>
             )}
 
-            {hasLinks && !menuHidden && (
-              <NavigationVisbilityObserver className={clsx(styles['SubdomainNavBar-primary-nav-list--visible'])}>
-                {menuItems}
-              </NavigationVisbilityObserver>
-            )}
-
-            <div
-              className={clsx(
-                styles['SubdomainNavBar-button-area'],
-                !menuHidden && styles['SubdomainNavBar-button-area--visible'],
-              )}
-            >
-              <div className={styles['SubdomainNavBar-button-area-inner']}>
-                {React.Children.toArray(children)
-                  .map(child => {
-                    if (React.isValidElement(child) && typeof child.type !== 'string') {
-                      if (child.type === PrimaryAction) {
-                        return child
+            {isMedium && (
+              <div
+                className={clsx(styles['SubdomainNavBar-button-area'], styles['SubdomainNavBar-button-area--visible'])}
+              >
+                <div className={styles['SubdomainNavBar-button-area-inner']}>
+                  {React.Children.toArray(children)
+                    .map(child => {
+                      if (React.isValidElement(child) && typeof child.type !== 'string') {
+                        if (child.type === PrimaryAction) {
+                          return child
+                        }
+                        return null
                       }
-                      return null
-                    }
-                  })
-                  .filter(Boolean)}
-
-                {React.Children.toArray(children)
-                  .map(child => {
-                    if (React.isValidElement(child) && typeof child.type !== 'string') {
-                      if (child.type === SecondaryAction) {
-                        return child
+                    })
+                    .filter(Boolean)}
+
+                  {React.Children.toArray(children)
+                    .map(child => {
+                      if (React.isValidElement(child) && typeof child.type !== 'string') {
+                        if (child.type === SecondaryAction) {
+                          return child
+                        }
+                        return null
                       }
-                      return null
-                    }
-                  })
-                  .filter(Boolean)}
+                    })
+                    .filter(Boolean)}
+                </div>
               </div>
-            </div>
+            )}
+
+            {!isMedium && (
+              <div
+                className={clsx(
+                  styles['SubdomainNavBar-menu-wrapper'],
+                  menuHidden && styles['SubdomainNavBar-menu-wrapper--close'],
+                )}
+              >
+                {hasLinks && !menuHidden && (
+                  <NavigationVisbilityObserver
+                    showOnlyOnNarrow
+                    className={clsx(styles['SubdomainNavBar-primary-nav-list--visible'])}
+                  >
+                    {menuItems}
+                  </NavigationVisbilityObserver>
+                )}
+                <div
+                  className={clsx(
+                    styles['SubdomainNavBar-button-area'],
+                    styles['SubdomainNavBar-button-area--visible'],
+                  )}
+                >
+                  <div className={styles['SubdomainNavBar-button-area-inner']}>
+                    {React.Children.toArray(children)
+                      .map(child => {
+                        if (React.isValidElement(child) && typeof child.type !== 'string') {
+                          if (child.type === PrimaryAction) {
+                            return child
+                          }
+                          return null
+                        }
+                      })
+                      .filter(Boolean)}
+
+                    {React.Children.toArray(children)
+                      .map(child => {
+                        if (React.isValidElement(child) && typeof child.type !== 'string') {
+                          if (child.type === SecondaryAction) {
+                            return child
+                          }
+                          return null
+                        }
+                      })
+                      .filter(Boolean)}
+                  </div>
+                </div>
+              </div>
+            )}
           </div>
         </div>
       </header>

diff --git a/packages/react/src/SubdomainNavBar/SubdomainNavBar.visual.spec.ts b/packages/react/src/SubdomainNavBar/SubdomainNavBar.visual.spec.ts
@@ -72,6 +72,19 @@ test.describe('Visual Comparison: SubdomainNavBar', () => {
     })
   })
 
+  // eslint-disable-next-line i18n-text/no-en
+  test.describe('Mobile viewport test for Mobile Menu Open Many Items', () => {
+    test.use({viewport: {width: 360, height: 800}})
+    test('SubdomainNavBar / Mobile Menu Open Many Items', async ({page}) => {
+      await page.goto(
+        'http://localhost:6006/iframe.html?args=&id=components-subdomainnavbar--mobile-menu-open-many-items&viewMode=story',
+      )
+
+      await page.waitForTimeout(5500)
+      expect(await page.screenshot()).toMatchSnapshot()
+    })
+  })
+
   // eslint-disable-next-line i18n-text/no-en
   test.describe('Mobile viewport test for Mobile Search Results Visible', () => {
     test.use({viewport: {width: 360, height: 800}})

diff --git a/...avBar-Mobile-view-16bce-le-No-Links-SubdomainNavBar-Mobile-No-Links-1-linux.png b/...avBar-Mobile-view-16bce-le-No-Links-SubdomainNavBar-Mobile-No-Links-1-linux.png
diff --git a/...avBar-Mobile-view-8d921-for-Mobile-View-SubdomainNavBar-Mobile-View-1-linux.png b/...avBar-Mobile-view-8d921-for-Mobile-View-SubdomainNavBar-Mobile-View-1-linux.png
diff --git a/...avBar-Mobile-view-98fdb-SubdomainNavBar-Mobile-Menu-Open-Many-Items-1-linux.png b/...avBar-Mobile-view-98fdb-SubdomainNavBar-Mobile-Menu-Open-Many-Items-1-linux.png
diff --git a/...avBar-Mobile-view-e88bc--Menu-Open-SubdomainNavBar-Mobile-Menu-Open-1-linux.png b/...avBar-Mobile-view-e88bc--Menu-Open-SubdomainNavBar-Mobile-Menu-Open-1-linux.png