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

update postStreamScrubber #1944

Closed
wants to merge 1 commit into from
Closed

update postStreamScrubber #1944

wants to merge 1 commit into from

Conversation

w-4
Copy link
Contributor

@w-4 w-4 commented Nov 27, 2019

fixes the scrubber in the case that posts have margins

Fixes #0000

Fixes the scrubber in case posts have margins.

Changes proposed in this pull request:

The check "top <= viewportTop" is not necessary. it's sufficient to take the index from the first item that passed the previous checks. it's the postStream-item that is first visible in the viewport.

Reviewers should focus on:

Screenshot

Confirmed

  • Frontend changes: tested on a local Flarum installation.

fixes the scrubber in the case that posts have margins
@stale
Copy link

stale bot commented Feb 26, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We do this to keep the amount of open issues to a manageable minimum.
In any case, thanks for taking an interest in this software and contributing by opening the issue in the first place!

@stale stale bot added the stale Issues that have had over 90 days of inactivity label Feb 26, 2020
Copy link
Sponsor 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.

Looks good to me.

@stale stale bot removed the stale Issues that have had over 90 days of inactivity label Mar 27, 2020
@dsevillamartin
Copy link
Member

@w-4 Can you provide an additional explanation on what this PR solves and how/why it solves it? Thanks 🙂.

@w-4
Copy link
Contributor Author

w-4 commented Apr 11, 2020

@datitisev To be honest, I don't really remember. I quickly tested it locally with and without the fix.
and for me it fixes the following behavior:
go to https://discuss.flarum.org/d/187-word-association-game/1797
press F5 (refresh the page) -> each time it jumps to a different post.

might be related to:
#2088

Edit:
I checked again, it didn't fix it. So not really sure what it does. But I'll leave it in, doesn't hurt.

@askvortsov1
Copy link
Sponsor Member

Thank you for the PR! I'm going to close this for now, we can always revisit it from #2088.

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.

3 participants