-
Notifications
You must be signed in to change notification settings - Fork 8.5k
New navigation scroll into view secondary item #234543
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.
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
New navigation scroll into view secondary item #234543
Conversation
This caused a failure on main, likely due to a PR being merged around the same time as elastic#229868, which removes the usage of the translation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non-blocking suggestion:
The current implementation is absolutely fine. What we can do to make it better is encapsulate it in a reusable hook:
export const useScrollToActive = (condition?: boolean) => {
const ref = useRef<HTMLElement>(null);
useEffect(() => {
if (condition && ref?.current) {
activeItemRef.current.scrollIntoView({
behavior: 'smooth',
block: 'start',
});
}
}, [condition]);
return ref;
}and then we get a cleaner logic in here:
export const SecondaryMenuItemComponent = ({
// ...
}) => {
// ...
const activeItemRef = useScrollToActive(isActive);
// ...
return (
<li ref={activeItemRef>
// ...
)
}|
@ek-so The smooth scrolling when a side panel opens feels like it makes sense. But for popovers, since the interaction is on hover, the smooth scroll looks a bit weird to me. Personally, I’d make the scroll immediate 🤔 Curious what you think! |
Thanks @paulinashakirova, @weronikaolejniczak! |
|
@ek-so @paulinashakirova personally, I would also add a timeout to the smooth scroll for the side panel. It could be 50 ms. I think if the side panel opens and the smooth scroll starts immediately, it'll be too much. LMK! |
I agree that the animation itself might feel smoother, but I would not add delays here tbh. My worry is that in this case it might feel like a slowly working system. And now it is smooth and quick at the same time. |
|
@ek-so @weronikaolejniczak Screen.Recording.2025-09-12.at.18.24.54.mov |
|
@paulinashakirova I think this is absolutely fine. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
Could you move this to the hooks folder? Also, it might be nice to add a JSDoc comment to explain the hooks purpose, the args it accepts and what it returns 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paulinashakirova If the use_scroll_to_active is used only inside the secondary_menu then we can leave it there. BUT it's so generic, it doesn't have to do with the secondary menu at all, that it's better placed inside that top-level hooks folder. I trust your judgement 😄 Thanks for adding the JSDoc!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right - mine is more generic, whereas popover is in the name of each hooks in that subfolder. 🙏🏻
|
@paulinashakirova @weronikaolejniczak I've been playing around for a while with those 2 options, and I came to a conclusion that keeping However, there are a couple of issues that seem to be independent from scroll behavior.
Could you pls look at those? |
|
@ek-so I reverted the scroll behaviour to be smooth, and also before the element scrolled into view, i tried to align to the most top position available, but now changed to minimal scroll distance, so if it appears from below it will stop at the bottom. Behaviour with popover Screen.Recording.2025-09-17.at.13.27.39.movBehaviour with side panel Screen.Recording.2025-09-17.at.13.39.08.mov |
|
@paulinashakirova secondary panel is used both in the popover and the side panel. It might be worth to test how the scrolling behaves with the side panel. Otherwise, looks good to me! 🟢 Amazing work! |
I updated my previous comment with a screen recording for the side panel. |
weronikaolejniczak
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's wait for Kate's approve but to me it's 👌🏻 👩🏻🍳
|
Thanks Paulina! Lgtm. |
weronikaolejniczak
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote a comment too soon but actually everything is correct here 🟢 Thanks for adding this!
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Page load bundle
History
|
## Summary This PR resolves [[SideNav] Smooth scroll to active element in side panel](elastic#234542) issue.
## Summary This PR resolves [[SideNav] Smooth scroll to active element in side panel](#234542) issue.
…lastic#237146) ## Summary This PR resolves [[SideNav] Smooth scroll to active element in side panel] (Initially this was introduced [here](elastic#234543)). (cherry picked from commit bfa3ee5)
## Summary This PR resolves [[SideNav] Smooth scroll to active element in side panel](elastic#234542) issue.
…lastic#237146) ## Summary This PR resolves [[SideNav] Smooth scroll to active element in side panel] (Initially this was introduced [here](elastic#234543)).
…lastic#237146) ## Summary This PR resolves [[SideNav] Smooth scroll to active element in side panel] (Initially this was introduced [here](elastic#234543)).

Summary
This PR resolves [SideNav] Smooth scroll to active element in side panel issue.
Screen.Recording.2025-09-10.at.11.05.17.mov