Add scrolling when dragging files. Fixes #12329

[ErikPel]

This should work but the numbers probably need tweaking so feedback is much appreciated.

Fixes #12329

[mention-bot]

By analyzing the blame information on this pull request, we identified @PVince81, @MorrisJobke and @butonic to be potential reviewers

[ErikPel]

Seems like I closed it by accident

[MorrisJobke]

@ErikPel Awesome! I added a tiny change to fix an issue with small screen (below 300 pixel in height - it was jumping). And now everything works like a charm.

Tested in Safari, Firefox, Chrome, IE11, Edge. 👍

@PVince81 @owncloud/designers Check this out! :)

[MorrisJobke]

cc @dttpy @SergioBertolinSG

[PVince81]

It's too sensitive and scrolls too early for me in Chrome:

[PVince81]

I expect it to only scroll when I reach the top item (or maybe the second top item), not the middle of the page.

[ErikPel]

Better or worse now @PVince81 ?
Also removed the jquery animate functions. Feels more responsive now. Not sure if others agree though

[PVince81]

Looks good now 👍

However it doesn't work on the public link page, might need a separate fix ?

[MorrisJobke]

This looks like broken syntax.

[ErikPel]

@MorrisJobke oh damn. Not sure what I was thinking there. Just quickly edited the file to match my own on the githubs browser editor and messed up. I'll fix that asap

[ErikPel]

@PVince81 Good catch.

It doesn't work because the public link page uses #content-wrapper id and the files app page uses #app-content

Easy fix would be to add css class to both of them and then select based on that? Should I do that?

[PVince81]

@ErikPel you can also use FileList.$container to find out which element is used for scrolling

[ErikPel]

@PVince81 Did not know that existed. Fixed now.

[PVince81]

I also just noticed this. Please don't decrement/increment inline. Put currentScrollTip -= 10; on a separate row if needed, for better readability.

[ErikPel]

@PVince81 not sure if I understood you correctly. Changed it. Was that what you meant?

[ErikPel]

@PVince81 makes sense. Added.

[PVince81]

No. Here you are still doing two things at the same time.
Please split it to two rows:

• row 1 decrements currentScrollTop
• row 2 sets it to the scrollTop(currentScrollTop) function.

Also one thing I don't understand is why you are using -= here instead of just - ?
You are not reusing currentScrollTop later on, so no need to update the variable's value.

[ErikPel]

        if (event.pageY < top){
            currentScrollTop -= 10
            $(scrollingArea).scrollTop(currentScrollTop);

        }
        else if (event.pageY > bottom) {
            currentScrollTop += 10
            $(scrollingArea).scrollTop(currentScrollTop);

        }

Is this what you mean? @PVince81

[PVince81]

Yes.

But then comes my second question, why increment/decrement currentScrollTop if you don't use it later ?

This would be enough:

   if (event.pageY < top){
            $(scrollingArea).scrollTop(currentScrollTop - 10);
        }
        else if (event.pageY > bottom) {
            $(scrollingArea).scrollTop(currentScrollTop + 10);
        }

[ErikPel]

@PVince81 I really have no idea. Guess I wasn't thinking.
Changed it now

[PVince81]

Seems worse now, maybe the scroll container is still the wrong one. No idea why:

[ErikPel]

Yeah seems like it. @PVince81
Seems like those animations were neccessary after all because they seem to fix the problem (or am I imagining things?).

[jancborchardt]

@ErikPel @PVince81 I would move this to 9.1 since we’re way past feature freeze and this has too much potential for breakage.

[PVince81]

Agreed

[PVince81]

@ErikPel any chance to finish this for 9.1 ?

[ErikPel]

@PVince81 I'm really busy with work right now but I can take look if you tell me what needs to be fixed.
If I remember correctly 2e4c033 was never tested since this was moved to 9.1.

[PVince81]

Ah, right. I didn't see that you added a commit.
In general it is good practice to post a message every time you make a change so people get notifications 😄

I'll retest it later then. Thanks!

[MorrisJobke]

I tested this again on a rebased version of this (I pushed this - sorry for highjacking, but this was 1,5k commits behind master:P)

Still works 👍

[PVince81]

Looks good now 👍
Note that when it scrolls down sometimes my nav bar shifts a little bit, but no biggie.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.