fix(#3548): pin secondary menu at bottom of Work Side Menu during scroll#3709
fix(#3548): pin secondary menu at bottom of Work Side Menu during scroll#3709
Conversation
Spark450
added
the
P3
twjeffery
force-pushed
the
tom/side-menu-scroll
branch
2 times, most recently
from
9c3723b to
2ce89d2
Compare
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.
twjeffery
force-pushed
the
tom/side-menu-scroll
branch
from
2ce89d2 to
96ecc0c
Compare
bdfranck
left a comment
bdfranck
left a comment
There was a problem hiding this comment.
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-sectionfor consistency - Remove bottom padding on primary menu to get rid of the extra space
- Use tokens for new values (GovAlta/design-tokens#144)
| observer = watchPathChanges(setCurrentUrl); | ||
|
|
||
| if (typeof ResizeObserver !== "undefined") { | ||
| _resizeObserver = new ResizeObserver(() => setMenuScrolling()); |
There was a problem hiding this comment.
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.
|
|
||
| // Menu | ||
| function updateScrollPosition() { | ||
| const innerEl = _scrollEl?.shadowRoot?.querySelector(".goa-scrollable"); |
There was a problem hiding this comment.
I'm not sure if this will work or not. But the Scrollable.svelte dispatches _scroll event that I think should be used for this instead if we can. Otherwise we're linked to the component in a more brittle fashion. You can reference Drawer.svelte on line 181 for how it's used there.
|
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 |
4 participants
Summary
Screen.Recording.2026-03-30.at.3.48.30.PM.mov
Top of scroll
Closes #3548
Test plan
/bugs/3548which has enough primary items to overflow