theodore-norvell / PLAAY

Senior Design Project PLAAY (Programming Language for Adults And Youth)
2 stars 0 forks source link

Selection scroll #66

Closed jillhancock closed 7 years ago

jillhancock commented 7 years ago

@inexpensive @WNewhook @christopher-rodgers Try some manual testing and report back!

WNewhook commented 7 years ago

@jillhancock This seems like it works for mouse navigation, however it doesn't play nice with keyboard navigation. It seems to cache scroll operations and only begin the next one when the previous one has finished. The effect is that if you create a really long sequence of nodes and hold down an arrow key it will slowly scroll in small, discrete chunks until it hits its destination instead of smoothly following the selection. Also, when scrolling down the scrolling stops slightly before I would prefer it to. This isn't a huge issue but I figured I'd point it out.

jillhancock commented 7 years ago

@WNewhook I didn't notice that it was caching the scrolls with keyboarding, good catch. And you're right about it stopping the scroll too early, that's an easy fix though.

jillhancock commented 7 years ago

Added a few fixes to the problems William pointed out:

WNewhook commented 7 years ago

@jillhancock Looks good with the new changes. I still think the scrolling stops a little too early when scrolling down, though. Maybe try using visibleHeight + 25 instead of +12? I played with the numbers a bit and I think that would look better.

jillhancock commented 7 years ago

@WNewhook Visible height + 25 gives me this:

scrolloffset25

Is that what you're seeing too? It gives an indication that there's a node beneath it which is nice, I didn't think of that!

WNewhook commented 7 years ago

@jillhancock This is what I'm seeing with +25:

plaayscreenshotscrolling

There is a node below the selected one. I don't know why I have a scroll bar and you don't, but it's there in both Firefox and Chrome. At +12 the selection was partially offscreen. I guess the problem is the browser considers the area hidden by the scroll bar to still be visible.

jillhancock commented 7 years ago

It should be fixed after that last commit. Just added the width of the scrollbar to the offset (0 for macOS/non visible ones). Thanks!

WNewhook commented 7 years ago

@jillhancock Perfect. Looks good now.

jillhancock commented 7 years ago

@christopher-rodgers This only seems to happen when you drag a new node from the sidebar, not when you move an already created node. It happens in master too so it's a bug that's been there for a while I think. I'll find a fix, but maybe it would fit better in issue #50?

inexpensive commented 7 years ago

I found an issue with embedded declaration nodes that I made a quick video describing.

EDIT: I should add the link

https://youtu.be/gzE_lksiOpU

jillhancock commented 7 years ago

@inexpensive Thanks for finding that one! When a node/drop zone was low enough in the container that it was obscured by a visible scrollbar, but not quite low enough to cross bottom edge of the main container, it didn't trigger a scroll. I didn't account for this when I added the scrollbar offset. Should be working now if you want to test it again.