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

Improve sticky scroll #262

Merged
merged 1 commit into from
Apr 29, 2016
Merged

Improve sticky scroll #262

merged 1 commit into from
Apr 29, 2016

Conversation

xPaw
Copy link
Member

@xPaw xPaw commented Apr 17, 2016

  • No longer overrides $.fn.append
  • No longer jumps to bottom whenever resizing the window
    • I tried making this keep the scroll position at lowest visible message, but there is no sane way of doing this
  • No longer sets the css with js, we already have it in our styles
  • Whenever you send the message, it will scroll to the bottom
  • Gives 30 pixels of slack to keep the scroll at the bottom
  • There's a stuckToBottom variable which can be used to display "More messages below" div in the future.

@xPaw xPaw added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label Apr 17, 2016
return false;
}
var el = this[0];
this.scrollTop(this[0].scrollHeight);
Copy link
Member

Choose a reason for hiding this comment

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

Crashes on private instances with Uncaught TypeError: Cannot read property 'scrollHeight' of undefined.

Seems like .sticky() is called on an empty jQuery set. Probably should loop over it since this is how jQuery recommends to do it, so we don't get surprises later if we try to sticky multiple things at once.

a.fn.scrollBottom   @   stickyscroll.js:28
a.fn.sticky @   stickyscroll.js:21
(anonymous function)    @   lounge.js:670
n.event.dispatch    @   jquery.js:4409
r.handle    @   jquery.js:4095
n.event.trigger @   jquery.js:4324
(anonymous function)    @   jquery.js:4875
n.extend.each   @   jquery.js:375
n.fn.n.each @   jquery.js:139
n.fn.extend.trigger @   jquery.js:4874
n.fn.(anonymous function)   @   jquery.js:7461
(anonymous function)    @   lounge.js:105
Emitter.emit    @   socket.io.js:5657
Socket.onevent  @   socket.io.js:5183
Socket.onpacket @   socket.io.js:5141
(anonymous function)    @   socket.io.js:5522
Emitter.emit    @   socket.io.js:5657
Manager.ondecoded   @   socket.io.js:4668
(anonymous function)    @   socket.io.js:5522
Emitter.emit    @   socket.io.js:2556
Decoder.add @   socket.io.js:6156
Manager.ondata  @   socket.io.js:4658
(anonymous function)    @   socket.io.js:5522
Emitter.emit    @   socket.io.js:2556
Socket.onPacket @   socket.io.js:459
(anonymous function)    @   socket.io.js:276
Emitter.emit    @   socket.io.js:2556
Transport.onPacket  @   socket.io.js:892
callback    @   socket.io.js:1766
(anonymous function)    @   socket.io.js:3558
exports.decodePayloadAsBinary   @   socket.io.js:3557
exports.decodePayload   @   socket.io.js:3325
Polling.onData  @   socket.io.js:1770
(anonymous function)    @   socket.io.js:1330
Emitter.emit    @   socket.io.js:2556
Request.onData  @   socket.io.js:1491
Request.onLoad  @   socket.io.js:1572
xhr.onreadystatechange  @   socket.io.js:1444

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@maxpoulin64
Copy link
Member

👍

@astorije
Copy link
Member

Overall, it works great, very good job @xPaw! A couple comments:

  • No longer jumps to bottom whenever resizing the window
    • I tried making this keep the scroll position at lowest visible message, but there is no sane way of doing this

I like that, it also solves it in another case: when you zoom in/out using Ctrl++/-, which is great in different situations.

I'm wondering though, is your attempt (the one in italic) still in the code (I'm thinking it's here)? Reason I ask is because the default behavior from browsers (well, at least with Chrome playing with it now) seems good enough, and at least similar to the result of your PR. After all, browsers have to deal with keeping text position on potentially very long documents (during resize or zoom in/out) so I'm fine leaving that decision to them. Even if they happen to not do it perfectly, it's the job of the browser so we can assume it'll get better over time. Just my 2c :-)
EDIT: Turns out lastHeight isn't used anywhere so that might be a leftover from your attempts.

Whenever you send the message, it will scroll to the bottom

I actually think this is better as it is right now. One use case, which can happen a great deal on IRC, is to log in and see lots of backlog to read. Users can currently scroll back, read, answer as they read without having their scroll affected. I think it should stay that way.
One way to improve that is to use that stuckToBottom variable and proposition to display a "More messages below" button in the future so that user can decide whenever they want to scroll to bottom. Does that make sense?
(I'm trying to keep personal feelings out of usability discussions, but I know that would affect me a lot.)

@astorije astorije self-assigned this Apr 28, 2016
@xPaw
Copy link
Member Author

xPaw commented Apr 28, 2016

Turns out lastHeight isn't used anywhere so that might be a leftover from your attempts.

Yep left over.

I actually think this is better as it is right now.

Removed the jump.

@astorije
Copy link
Member

👍 and merging.

@astorije astorije merged commit d171e3c into master Apr 29, 2016
@astorije astorije deleted the xpaw/sticky branch April 29, 2016 04:15
@astorije astorije added this to the ★ Next Release milestone May 15, 2016
matburnham pushed a commit to matburnham/lounge that referenced this pull request Sep 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature Tickets that describe a desired feature or PRs that add them to the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants