-
Notifications
You must be signed in to change notification settings - Fork 3
fix v2 drawer scrolling issues #1908
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
Conversation
6102c38
to
25cd6eb
Compare
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.
Works well. One question for you about possible simplification.
const outerContainerRef = useRef<HTMLDivElement>(null) | ||
const searchParams = useSearchParams() | ||
const searchParamsRef = useRef(searchParams) | ||
useEffect(() => { |
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.
Would this work instead?
const outerContainerRef = useRef<HTMLDivElement>(null)
useEffect(() => {
if (outerContainerRef.current) {
outerContainerRef.current.scrollTo(0, 0)
}
}, [resource?.id])
Only possible quirk i see is that if you scroll while the resource is still loading, it will scroll up when when the resource finishes loading.
If that seems undesirable, maybe pass the ID into v2expanded as a prop. (Alternatively the effect could run when searchParams.get("resource")
changes, and eliminate the search params ref, but currently v2expanded has no knowledge of routing, e.g., it doesn't know the "resource" querykey is involved at all, mildly nice to presreve that..)
@ChristopherChudzicki Thanks, this is ready for another look now |
What are the relevant tickets?
Closes https://github.com/mitodl/hq/issues/6293
Description (What does it do?)
This PR fixes various scrolling related to the new learning resource drawer. Firstly, clicking the "Show more" link on the description or the start dates were causing the main window to scroll to the top. This is caused by the
scroll
property onNextLink
: https://nextjs.org/docs/pages/api-reference/components/link#scroll. It defaults totrue
, so whenever you click aNextLink
,nextjs
scrolls the window to the top.scroll
can now be passed as a boolean argument to ourLink
component, which passes it on to theNextLink
component. For the links inside the drawer, we passfalse
which prevents the scroll to the top from happening.The second scrolling option here has to do with the cards in the recommendation carousel. Currently clicking them reloads the drawer, but does not scroll the drawer to the top. In order to fix this, code was added to
LearningResourceExpandedV2
to reset the scroll position if the resource ID in the query string has changed.How can this be tested?
lr_drawer_v2
flagdrawerV2
to betrue
inLearningResourceDrawer.tsx
LearningResourceDrawerV2
, temporarily setcarousels={[similarResourcesCarousel, similarResourcesCarousel, similarResourcesCarousel]}
to create enough scrollable content to demonstrate the second issue detailed abovemit-learn