Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Fix scroll-jumps when jumping to a permalink #9340

Closed
wants to merge 5 commits into from
Closed

Fix scroll-jumps when jumping to a permalink #9340

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c9198f6
Fix scroll-jumps when jumping to a permalink
ara4n Oct 2, 2022
1335e28
Merge branch 'develop' into matthew/fix-scroll-jumps-on-permalink
MadLittleMods Nov 16, 2022
39c0112
Merge branch 'develop' into matthew/fix-scroll-jumps-on-permalink
Johennes Nov 17, 2023
d96a42d
Merge branch 'develop' into matthew/fix-scroll-jumps-on-permalink
t3chguy Nov 17, 2023
04a8d46
Merge branch 'develop' into matthew/fix-scroll-jumps-on-permalink
dbkr Feb 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion src/components/structures/ScrollPanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,15 @@ export default class ScrollPanel extends React.Component<IProps> {

private onScroll = ev => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's ship this 🙇

Merged develop so we can ensure it's still good to go test wise even though there are no scroll tests.

Copy link
Contributor

@MadLittleMods MadLittleMods Nov 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

TypeScript strict CI job is failing but it's complaining about an adjacent unrelated line of code and that job is not required to pass ✅

Cypress flake that passed after the second run ✅

Everything looks okay to merge ⏩

// skip scroll events caused by resizing
if (this.props.resizeNotifier && this.props.resizeNotifier.isResizing) return;
if (this.props.resizeNotifier && this.props.resizeNotifier.isResizing) {
debuglog("skipping onScroll due to isResizing", this.getScrollNode().scrollTop);
return;
}
// skip scroll events which are triggered by backfilling (i.e. while loading permalinks)
if (this.isFilling) {
debuglog("skipping onScroll due to isFilling", this.getScrollNode().scrollTop);
return;
}
Copy link
Contributor

@MadLittleMods MadLittleMods Nov 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

we're not sure if this actually fixes an issue at the moment, and needs a regression test to do so.

@ara4n thinks the problem might still be reproducible even with this fix but do these changes at least make sense? There could be many points that could cause the problem and this plugs a few of the holes.

In that case, we should merge anyway

debuglog("onScroll", this.getScrollNode().scrollTop);
this.scrollTimeout.restart();
this.saveScrollState();
Expand Down