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

Fix DiscussionListPane jumping around (when going from discussion to discussion) #2402

Merged
merged 4 commits into from
Jan 13, 2021

Conversation

w-4
Copy link
Contributor

@w-4 w-4 commented Oct 21, 2020

Fixes the following issue:

  1. Go to https://nightly.flarum.site and open a discussions.
  2. On the discussion page scroll the pane a little.
  3. Click on a different discussion and the pane jumps back to the top, not keeping the scrolled position.

Confirmed

  • Frontend changes: tested on a local Flarum installation.

(when going from discussion to discussion)
js/src/forum/components/DiscussionListPane.js Outdated Show resolved Hide resolved
js/src/forum/components/DiscussionListPane.js Outdated Show resolved Hide resolved
@w-4 w-4 requested a review from askvortsov1 October 25, 2020 01:23
Copy link
Member

@askvortsov1 askvortsov1 left a comment

Choose a reason for hiding this comment

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

Thank you!

@clarkwinkelmann
Copy link
Member

Is that paneScrollTop global variable exclusive to this particular feature? If so I think its name should reference the component that makes use of it, so we're certain it does not get used by something unrelated in the future or by extensions.

@w-4
Copy link
Contributor Author

w-4 commented Dec 12, 2020

I renamed the variable. Also, after implementing https://github.com/flarum/core/issues/2413 caching of the scroll position won't be necessary anymore.

@clarkwinkelmann
Copy link
Member

I haven't followed well enough. Will this still be necessary? Should I review again?

@askvortsov1
Copy link
Member

@w-4 Just to clarify, you're saying that with flarum/issue-archive#176 the change in this PR won't be needed, right? (since we won't be rerendering the panel completely anyways). If so, maybe we shouldn't be merging this?

@w-4
Copy link
Contributor Author

w-4 commented Jan 10, 2021

@askvortsov1 Yes, only the if condition if (!app.previous.isDiscussion()) would be needed. I'd merge it in, it fixes the bug, and flarum/issue-archive#176 can be done after stable.

@askvortsov1 askvortsov1 merged commit 2e3197d into flarum:master Jan 13, 2021
@askvortsov1 askvortsov1 added this to the 0.1.0-beta.16 milestone Jan 13, 2021
@askvortsov1 askvortsov1 removed this from the 0.1.0-beta.16 milestone Jan 29, 2021
imorland pushed a commit to giffgaff/flarum-core that referenced this pull request Jan 29, 2021
Ensure that scroll position is retained between page changes, so if navigating via the sidebar, you don't need to re-scroll down every time.
matteocontrini pushed a commit to fibraclick/flarum-framework that referenced this pull request Feb 18, 2021
Ensure that scroll position is retained between page changes, so if navigating via the sidebar, you don't need to re-scroll down every time.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants