Skip to content

Conversation

@jonkafton
Copy link
Contributor

What are the relevant tickets?

Closes https://github.com/mitodl/hq/issues/4741

Description (What does it do?)

Scrolls the top of the search results to the center of the page on pagination.

Screenshots (if appropriate):

Screen.Recording.2024-06-28.at.15.35.18.mov

How can this be tested?

Navigate to the search page or a unit page and paginate through search results.

@jonkafton jonkafton force-pushed the jk/4741-scroll-on-paginate branch from aa982ba to 937b322 Compare June 28, 2024 14:29
@jonkafton jonkafton added the Needs Review An open Pull Request that is ready for review label Jun 28, 2024
@rhysyngsun rhysyngsun self-assigned this Jun 28, 2024
Copy link
Contributor

@rhysyngsun rhysyngsun left a comment

Choose a reason for hiding this comment

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

Functionality looks good, just one piece of feedback.

setSearchParams,
}) => {
const [searchParams] = useSearchParams()
const scrollHook = useRef<HTMLInputElement>(null)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should just be HTMLElement because it's a div

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good spot, thanks.

Copy link
Contributor

@rhysyngsun rhysyngsun left a comment

Choose a reason for hiding this comment

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

LGTM

@rhysyngsun rhysyngsun added Waiting on author and removed Needs Review An open Pull Request that is ready for review labels Jun 28, 2024
Comment on lines +686 to +691
setTimeout(() => {
scrollHook.current?.scrollIntoView({
block: "center",
behavior: "smooth",
})
}, 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity... why wrap this in setTimeout?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Despite some investigating, all I can say for sure is that it doesn't work without it. Likely some nuance around the dom element not being available during the old render cycle.

@jonkafton jonkafton merged commit a12d52a into main Jun 28, 2024
@jonkafton jonkafton deleted the jk/4741-scroll-on-paginate branch June 28, 2024 17:46
This was referenced Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants