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

Notifications Dropdown infinite scrolling #5237

Merged
merged 2 commits into from Oct 14, 2014

Conversation

Projects
None yet
4 participants
@jaideng123
Copy link
Contributor

jaideng123 commented Sep 20, 2014

I don't think there was an issue for this but i felt compelled to implement it after working on issue #5106.
Whenever the user scrolls to the bottom of the notifications dropdown it will load 5 more notifications until there are none left to show

@Quix0r

This comment has been minimized.

Copy link

Quix0r commented Sep 28, 2014

Can this be added?

$('.notifications').scroll(function(e) {
var bottom = $('.notifications').prop('scrollHeight') - $('.notifications').height();
var currentPosition = $('.notifications').scrollTop();
if (currentPosition + 50 >= bottom && self.notifications.length % 5 === 0 && !isLoading) {

This comment has been minimized.

@svbergerem

svbergerem Sep 28, 2014

Member

Should self.notifications.length % 5 === 0 prevent loading more notifications when there are no additional notifications? That doesn't work when the number of total notificatons is divisible by 5. You could instead check if notificationsLoaded <= self.notifications.length.

@@ -54,7 +57,7 @@
};

this.getNotifications = function() {
$.getJSON("/notifications?per_page=15", function(notifications) {
$.getJSON("/notifications?per_page="+notificationsLoaded, function(notifications) {

This comment has been minimized.

@svbergerem

svbergerem Sep 28, 2014

Member

This would reload all notifications. The notifications controller allows us to get additional notifications by using the page parameter. It would be great if you could use that. Perhaps write a new function (this.getMoreNotifications?) which calls "/notifications?per_page=5&page="+page where page is initially set to 3 and will be increased by 1 every time you scroll down in the notifications badge.

@jaideng123

This comment has been minimized.

Copy link
Contributor

jaideng123 commented Sep 28, 2014

Sounds good! I'll start fixing it up

@jaideng123 jaideng123 force-pushed the jaideng123:added_infinite_scrolling_to_notedropdown branch from d9d42f8 to f1ad7ba Sep 29, 2014

@jaideng123 jaideng123 force-pushed the jaideng123:added_infinite_scrolling_to_notedropdown branch from f1ad7ba to 2b6ac92 Sep 29, 2014

@jaideng123

This comment has been minimized.

Copy link
Contributor

jaideng123 commented Sep 30, 2014

Alright, take a look

@jhass

This comment has been minimized.

Copy link
Member

jhass commented Sep 30, 2014

If you could add some tests that should be good to go :)

@jaideng123

This comment has been minimized.

Copy link
Contributor

jaideng123 commented Oct 4, 2014

Would this be a jasmine or an rspec test?

@jhass

This comment has been minimized.

Copy link
Member

jhass commented Oct 4, 2014

Jasmine, you added new functionality in JS :) If you can't come up with one a cucumber test would be okay too, though we already have too many of them ;)

@jaideng123

This comment has been minimized.

Copy link
Contributor

jaideng123 commented Oct 6, 2014

Alright tests are in

@jhass jhass added this to the next-major milestone Oct 14, 2014

@jhass jhass merged commit bd24d6b into diaspora:develop Oct 14, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

jhass added a commit that referenced this pull request Oct 14, 2014

Merge pull request #5237 from jaideng123/added_infinite_scrolling_to_…
…notedropdown

Notifications Dropdown infinite scrolling
@jhass

This comment has been minimized.

Copy link
Member

jhass commented Oct 14, 2014

Alright, let's try this out! Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment