kandanapp / kandan

Kandan is an Open Source Alternative to HipChat
GNU Affero General Public License v3.0
2.72k stars 408 forks source link

bug with scroll #419

Closed msslava closed 9 years ago

msslava commented 9 years ago

When I do scroll to the top, begins to fail scroll https://yadi.sk/i/3zvNeATZdWYep

scouttyg commented 9 years ago

I'll take a look!

hellbinder commented 9 years ago

This worked for me

In app/assets/javascripts/backbone/views/channel_pane.js just added

if collection.length > 0

right after the success callback.

So the whole fetch would look like

activities.fetch
  data: { oldest: oldest },
  success: (collection) =>
    if collection.length > 0
      for activity in collection.models.reverse()
        activityView = new Kandan.Views.ShowActivity(activity: activity, silence_mentions: true, silence_music: true)
        $container.find(".channel-activities").prepend(activityView.render().el)

      if $current_top_element.length != 0
        $container.scrollTop($current_top_element.offset().top)

      Kandan.Helpers.Channels.setPaginationState(
        collection.channelId,
        collection.moreActivities,
        _.last(collection.models),
        $container
      )

      @loading_new_messages = false

Have a branch with this fix if it looks right. First post ever here so if I'm doing something wrong let me know.

hellbinder commented 9 years ago

Also...thoughts on creating a button to load previous activities instead of auto scroll (like Iphone messages). If for some reason the paging option is set to a number that will cause the activity with limit to already fit your screen then you won't have a scroll bar and so you won't be able to load previous activities.

scouttyg commented 9 years ago

@hellbinder your fix looks good, feel free to add it to a branch and I'll pull it in.

For the second thought, this should actually be fixed by line #12 in channel_pane.js (the same file you are talking about):

    if !$paginated_activities.hasScrollBar()
      @loading_new_messages = true;
      @loadMoreActivities($container)
hellbinder commented 9 years ago

Hi again. Forgot to mention something (rookie mistake). Will try to explain before trying to do a pull request. The above example worked because I had also changed oldest variable in channel_pane.js Instead of

oldest = $pagination.data("oldest")

I changed it to

oldest = $current_top_element.prop("id").split("-")[1] #get first id from activity in DOM

This works perfectly but it might be hacky? so I just went ahead and dug further and after while noticed that leaving everything as it was and changing the file in app/assets/javascripts/backbone/helpers/channels.js.coffee and moving the line

# Only set pagination data if there are more activities. Otherwise is useless
@channelPaginationEl(channelId).data("oldest", oldest.get("id"))

outside of the conditional statement would also make it work as expected though it will give a warning when oldest becomes undefined If it's left as is it was, it will keep reloading the last activities because the oldest variable will not change to the correct oldest variable once it's reaching the end of the activities.

Any ideas/suggestions?

Created two branches to separate the two solutions.

https://github.com/hellbinder/kandan

scouttyg commented 9 years ago

I think you had it right initially -- you only need do the collection.length piece, not anything else. Or was this talking about another bug?

Before:

first

After:

second

hellbinder commented 9 years ago

It seems to me that the oldest variable does not change when its getting to close the the first activity id in the channel. That's why I changed it by looking up the first activityid in the DOM iitself$current_top_element.prop("id").split("-")[1]. Since it never gets to the correct first activityid (the oldest) it will keep reloading. See the GIF and pay attention to the console section to see if that makes sense. Only change in there is with the collection.length > 0 conditional.

kandan_scroll_bug

hellbinder commented 9 years ago

Does it work for you by just adding the collection.length > 0 line?

scouttyg commented 9 years ago

@hellbinder Ah I see, yep I think you got it right, so it would be something like:

helpers/channels.js.coffee

  @setPaginationState: (channelId, moreActivities, oldest) ->
    console.log "pagination element", moreActivities, @channelPaginationEl(channelId)
    @channelPaginationEl(channelId).data("oldest", oldest.get("id"))

    if moreActivities == true
      @channelPaginationEl(channelId).show()
    else
      @channelPaginationEl(channelId).hide()

      # If there are no more messages we will unbind the scroll event
      @channelPane(channelId).unbind("scroll")

And:

views/channel_pane.js.coffee

    activities.fetch
      data: { oldest: oldest },
      success: (collection) =>
        if collection.length > 0
          for activity in collection.models.reverse()
            activityView = new Kandan.Views.ShowActivity(activity: activity, silence_mentions: true, silence_music: true)
            $container.find(".channel-activities").prepend(activityView.render().el)

          if $current_top_element.length != 0
            $container.scrollTop($current_top_element.offset().top)

          Kandan.Helpers.Channels.setPaginationState(
            collection.channelId,
            collection.moreActivities,
            _.last(collection.models),
            $container
          )

        @loading_new_messages = false

Feel free to push that pull request up and we can pull that in!

hellbinder commented 9 years ago

I pulled in my changes. Is there anything else I need to do for this? Wondering since it's been a week since my pull request. :smile:

hellbinder commented 9 years ago

New pull request. Hopefully it's all good.