Skip to content

fix(#3548): pin secondary menu at bottom of Work Side Menu during scroll#3709

Open
twjeffery wants to merge 1 commit intodevfrom
tom/side-menu-scroll
Open

fix(#3548): pin secondary menu at bottom of Work Side Menu during scroll#3709
twjeffery wants to merge 1 commit intodevfrom
tom/side-menu-scroll

Conversation

@twjeffery
Copy link
Copy Markdown
Collaborator

@twjeffery twjeffery commented Mar 30, 2026 •
edited
Loading

Summary

  • Restructures the Work Side Menu so only the primary nav area scrolls, while the secondary menu (Search, Get support, Release notes), account section, and collapse toggle stay pinned at the bottom
  • Adds scroll-aware border and shadow indicators that fade in/out (300ms) based on scroll position, only showing when there is hidden content in that direction
  • Adds browser test verifying secondary menu remains visible when primary nav overflows
Screen.Recording.2026-03-30.at.3.48.30.PM.mov

Top of scroll

image ### Middle of scroll image ### Bottom of scroll image ### No scroll image

Closes #3548

Test plan

  • Open the PR playground at /bugs/3548 which has enough primary items to overflow
  • Verify primary nav scrolls independently while secondary menu stays pinned at bottom
  • Verify header stays fixed at top, items don't scroll behind it
  • Verify border and shadow appear/fade when scrolled away from top or bottom
  • Verify border and shadow disappear when scrolled fully to either end
  • Collapse the menu and verify layout still works at 72px width
  • Resize the viewport height and confirm the layout adapts
  • Test on mobile viewport (menu opens as full-screen modal)

@Spark450 Spark450 added the P3 Priority 3 (nice to have): Valuable, but safe to release after launch. label Mar 30, 2026
ArakTaiRoth
ArakTaiRoth reviewed Apr 1, 2026
View reviewed changes
@twjeffery twjeffery force-pushed the tom/side-menu-scroll branch 2 times, most recently from 9c3723b to 2ce89d2 Compare April 2, 2026 19:05
@twjeffery twjeffery requested a review from ArakTaiRoth April 2, 2026 19:06
@twjeffery twjeffery force-pushed the tom/side-menu-scroll branch from 2ce89d2 to 96ecc0c Compare April 2, 2026 19:21
@bdfranck bdfranck mentioned this pull request Apr 2, 2026
Merged
bdfranck
bdfranck reviewed Apr 2, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@bdfranck bdfranck left a comment

Choose a reason for hiding this comment

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

Looks good! I have a couple suggestions that I added in a commit. You can squash them if you agree. Changes include:

  • Give the top section the class name .top-section for consistency
  • Remove bottom padding on primary menu to get rid of the extra space
  • Use tokens for new values (GovAlta/design-tokens#144)

ArakTaiRoth
ArakTaiRoth reviewed Apr 7, 2026
View reviewed changes
libs/web-components/src/components/work-side-menu/WorkSideMenu.svelte Outdated
observer = watchPathChanges(setCurrentUrl);

if (typeof ResizeObserver !== "undefined") {
_resizeObserver = new ResizeObserver(() => setMenuScrolling());
Copy link
Copy Markdown
Collaborator

@ArakTaiRoth ArakTaiRoth Apr 7, 2026

Choose a reason for hiding this comment

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

Potential problem with this. The ResizeObserver fires setMenuScrolling, which has the potential to change the observed _menuEl, which would cause it to fire again. I can't come up with an actual test that will have this problem, just noticed it as a potential issue we may want to do something about now.

Copy link
Copy Markdown
Collaborator Author

@twjeffery twjeffery Apr 10, 2026

Choose a reason for hiding this comment

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

I wrapped the callback in performOnce with its own timer so it doesn't interfere with the existing one in addMenuLink, Which should prevent any re-triggering.

Copy link
Copy Markdown
Collaborator

@ArakTaiRoth ArakTaiRoth Apr 10, 2026

Choose a reason for hiding this comment

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

@twjeffery Not quite the same issue:

  1. ResizeObserver fires, line 84
  2. It schedules setMenuScrolling, line 85
  3. That function can change _isScrolling, line 111
  4. _isScrolling controls the scrolling class, line 349
  5. The .scrolling class can change the width, line 550
  6. The ResizeObserver in step 1 above, is watching _menuEl, which is inside that container, and thus can have its width change, which would refire ResizeObserver

@twjeffery
Copy link
Copy Markdown
Collaborator Author

twjeffery commented Apr 7, 2026

Quick update on this one @bdfranck @ArakTaiRoth:

I got the suggestions and review points sorted out locally and tests are still passing.

I reviewed and merged the token PR from Benji, but the npm release failed due to a bad token. Now waiting on another fix PR to re-trigger the publish before I squash everything together and push

@twjeffery twjeffery force-pushed the tom/side-menu-scroll branch from 2708b8a to 270bea5 Compare April 10, 2026 15:32
@netlify
Copy link
Copy Markdown

netlify bot commented Apr 10, 2026 •
edited
Loading

✅ Deploy Preview for benji-docs-previews ready!

Name Link
🔨 Latest commit eb3b476
🔍 Latest deploy log https://app.netlify.com/projects/benji-docs-previews/deploys/69d9546926e1da0008f7c1ea
😎 Deploy Preview https://deploy-preview-3709--benji-docs-previews.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@twjeffery twjeffery force-pushed the tom/side-menu-scroll branch from 270bea5 to dd7f364 Compare April 10, 2026 15:44
@twjeffery
Copy link
Copy Markdown
Collaborator Author

twjeffery commented Apr 10, 2026

@bdfranck @ArakTaiRoth Should be good to review again now

@twjeffery @bdfranck
fix(#3548): pin secondary menu at bottom of Work Side Menu during scroll
eb3b476 
Restructure the side menu layout so only the primary nav scrolls while
secondary menu, account section, and collapse toggle stay fixed at the
bottom. Adds scroll-aware border and shadow indicators that fade in
when content is hidden in either direction. Uses a ResizeObserver
(debounced via performOnce) to detect dynamic overflow from group
expansion or window resize, and caches scrollTop from goa-scrollable's
_scroll event to avoid shadow DOM coupling.

Co-authored-by: Benji Franck <1479091+bdfranck@users.noreply.github.com>
@twjeffery twjeffery force-pushed the tom/side-menu-scroll branch from dd7f364 to eb3b476 Compare April 10, 2026 19:49
bdfranck
bdfranck approved these changes Apr 10, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@bdfranck bdfranck left a comment

Choose a reason for hiding this comment

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

I looked at the latest changes...

  • ✅ Borders and shadows work as expected
  • ✅ There isn't extra space between the primary and secondary menus

Image

Looks good to me! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bdfranck bdfranck bdfranck approved these changes

@ArakTaiRoth ArakTaiRoth Awaiting requested review from ArakTaiRoth

At least 2 approving reviews are required to merge this pull request.

Assignees

No one assigned

Labels

P3 Priority 3 (nice to have): Valuable, but safe to release after launch.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Work Side Menu: Secondary menu should stay pinned at bottom during scroll

4 participants

@twjeffery @ArakTaiRoth @bdfranck @Spark450