Skip to content
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

Perform scrolling prior to Visit completion #476

Merged
merged 2 commits into from Jul 29, 2022

Conversation

seanpdoyle
Copy link
Contributor

@seanpdoyle seanpdoyle commented Nov 25, 2021

Closes #400

When processing a Visit, invoke Visit.performScroll() while loading
a response, loading a Snapshot, or executing a same-page anchor
navigation. After in-lining those calls to more appropriate points in
the Visit lifecycle, this commit removes the performScroll() call from
the Visit.render() method.

@seanpdoyle
Copy link
Contributor Author

This is a draft in an attempt to resolve the issues causing #400. If these changes fix the issue, I'll create a browser test and upgrade this to Ready for Review.

@seanpdoyle seanpdoyle force-pushed the synchronize-view-scroll branch 2 times, most recently from ef92095 to e828656 Compare November 26, 2021 17:08
@seanpdoyle seanpdoyle changed the title Delay Visit page scrolling Treat same-page anchor Visit like a request Nov 26, 2021
@seanpdoyle seanpdoyle changed the title Treat same-page anchor Visit like a request Treat same-page anchor Visit like a simulated request Nov 26, 2021
@seanpdoyle seanpdoyle force-pushed the synchronize-view-scroll branch 2 times, most recently from 33f562b to 8af1d0b Compare November 27, 2021 06:16
@seanpdoyle seanpdoyle changed the title Treat same-page anchor Visit like a simulated request Perform scrolling prior to Visit completion Nov 27, 2021
@dhh dhh added the scrolling label Jun 19, 2022
@seanpdoyle seanpdoyle marked this pull request as ready for review July 16, 2022 14:59
@seanpdoyle seanpdoyle changed the title Perform scrolling prior to Visit completion Improve scroll retention test coverage Jul 16, 2022
@seanpdoyle seanpdoyle changed the title Improve scroll retention test coverage Perform scrolling prior to Visit completion Jul 16, 2022
@seanpdoyle seanpdoyle force-pushed the synchronize-view-scroll branch 2 times, most recently from da86b09 to 89b0df7 Compare July 16, 2022 15:39
@seanpdoyle
Copy link
Contributor Author

@srt32 since this follows-up the changes you made in #571, would you be able to double-check that it doesn't reverse the behavior fixes or introduce new quirks?

@dhh dhh added this to the 7.2.0 milestone Jul 17, 2022
@petrsigut
Copy link

This is awesome!

We tested this branch and it fixes all problems we have in our chat app, where we need to scroll to the bottom after Turbo visit😇

Closes hotwired#400

When processing a `Visit`, invoke `Visit.performScroll()` while loading
a response, loading a Snapshot, or executing a same-page anchor
navigation. After in-lining those calls to more appropriate points in
the Visit lifecycle, this commit removes the `performScroll()` call from
the `Visit.render()` method.
Resolves a [flaky test][] by replacing a call to `nextBeat()` with a
helper that will wait until the target element (in this case,
`turbo-frame#frame`) dispatches a particular event (in this case,
`turbo:frame-load`).

[flaky test]: https://github.com/hotwired/turbo/runs/7570036350?check_suite_focus=true#step:12:16
@dhh dhh merged commit 4450843 into hotwired:main Jul 29, 2022
@seanpdoyle seanpdoyle deleted the synchronize-view-scroll branch July 29, 2022 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Anchor links appear at bottom of page instead of top
3 participants