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

Implement Scroll to Bottom on Timeline V2 #1493

Conversation

marcustyphoon
Copy link
Collaborator

@marcustyphoon marcustyphoon commented Jun 12, 2024

Description

This implements Scroll to Bottom on V2 timelines.

As noted in the linked issue, on V2 timelines, the loader element we need to detect so that we keep scrolling conditionally:

  • Isn't the last child of its parent anymore (and doesn't have a classname iirc?)
  • Doesn't exist unless the timeline is actively loading content

Thus, this:

  • Fixes the selector
  • Implements new logic: scrolling will stop after a short delay unless a loader is added during the delay window. (This is basically equivalent to "debounce the stopscrolling function" in some meaningful sense, though I believe literally doing that has some bad edge cases. Didn't think this through much; if that works we should do it so the code is easier to understand.)

Resolves #1491.

Merge conflicts spectacularly with #977, but what can you do, I guess. (#1492, is what.)

Testing steps

not formally tested as of this pr description update; should be tested on following/followers/activity as well as post timelines

edit: tested (works, stops at end) on:

  • following
  • followers
  • activity

@marcustyphoon marcustyphoon marked this pull request as ready for review June 29, 2024 05:56
Copy link
Owner

@AprilSylph AprilSylph left a comment

Choose a reason for hiding this comment

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

This PR is in need of a rename prior to merging.

(master branch) (#1492) would look a little silly in the changelog.

src/features/scroll_to_bottom.js Outdated Show resolved Hide resolved
@marcustyphoon marcustyphoon changed the title Implement Scroll to Bottom on Timeline V2 (master branch) Implement Scroll to Bottom on Timeline V2 Jul 13, 2024
Co-authored-by: April Sylph <28949509+AprilSylph@users.noreply.github.com>
@marcustyphoon
Copy link
Collaborator Author

This PR is in need of a rename prior to merging.

(master branch) (#1492) would look a little silly in the changelog.

Or a rename during the squash merge, but yep!

AprilSylph
AprilSylph previously approved these changes Jul 13, 2024
Copy link
Owner

@AprilSylph AprilSylph left a comment

Choose a reason for hiding this comment

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

Seems to work fine:

  • /dashboard: just kept scrolling, as it would
  • /blog/april-codes: all 304 posts loaded, then stopped scrolling

@AprilSylph
Copy link
Owner

Or a rename during the squash merge, but yep!

I don't like doing that unless it's my own PRs...

[Remembers I'm only reviewing your PRs, not merging them for you]

Ohhhhhh yeah...

@marcustyphoon
Copy link
Collaborator Author

Oh, grr, why does this not work on queue.

@AprilSylph
Copy link
Owner

Oh. Queue hasn't been migrated yet. It's probably gonna be one of the last timelines migrated, since the reordering logic.

@AprilSylph
Copy link
Owner

I tested this on a V1 timeline (/explore/answertime) and it worked there, though? Loaded everything, then stopped.

@marcustyphoon
Copy link
Collaborator Author

Yeah—I thought I just needed to add sortableContainer along with scrollContainer but that's not working, hold on.

@AprilSylph
Copy link
Owner

AprilSylph commented Jul 13, 2024

ooh, sortableContainer is a literal classname, not an SCSS one. You're not trying to feed it to keyToCss, are you?

@marcustyphoon
Copy link
Collaborator Author

it's WHAT

Co-Authored-By: April Sylph <28949509+AprilSylph@users.noreply.github.com>
@AprilSylph
Copy link
Owner

<div class="sortableContainer">

This might make more sense if you know that Redpop uses SortableJS v1. Or it might not! I've actually touched that code a lot (attempting to upgrade it to v2; I failed) and I don't remember why it's there or like that.

Copy link
Owner

@AprilSylph AprilSylph left a comment

Choose a reason for hiding this comment

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

I don't have a queue long enough to test the latest commit, but code LGTM, and I presume you tested it yourself.

@marcustyphoon
Copy link
Collaborator Author

Today I learned: I should probably make my CSS translation dev tool's output have more of a distinction between source and translated class names :D

@marcustyphoon marcustyphoon merged commit 2d1c989 into AprilSylph:master Jul 13, 2024
2 checks passed
@marcustyphoon marcustyphoon deleted the scroll-to-bottom-timeline-v2-master branch July 13, 2024 20:01
@AprilSylph
Copy link
Owner

The way the classnames are shown in development builds of Redpop are {fileName}-{sourceName}--{generatedName}. Most stylesheets are just named style.scss so this ends up easily replicable in most cases by just doing style-{sourceName}--{generatedName}. I ~recently updated my own code snippet to reflect this: https://gist.github.com/AprilSylph/511b809ffec1f4e3b8bcb4af93c1f8cd

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.

Scroll to Bottom breaks on some timelines; blog lists; activity
2 participants