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

util/scrollTo.js: add option to scroll selected item to the top of sc… #10

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

maps82
Copy link

@maps82 maps82 commented Aug 4, 2016

I added a parameter to the scrollTo function to scroll selected to the top of scrollParent, instead to the bottom. Would be great if that could be merged :)

@jquense
Copy link
Member

jquense commented Aug 4, 2016

I'm all for adding this, does this handle scrolling up from below the selected item with an inverse logic to the default? meaning will the selected item is below the viewport it scrolls just enough to pull it into view (ie the bottom) when the item is a one the viewport it scrolls to top. I'd imagine this would want to do the opposite in both cases?

@maps82
Copy link
Author

maps82 commented Aug 4, 2016

I'm not sure. I currently use it with a slightly modified version of react-widgets, to get the timepicker scroll automatically to a given time when opening it (as long is no selection is made)

Do you have a simple test scenario for me?

@jquense
Copy link
Member

jquense commented Aug 4, 2016

I'd just look at the behavior in RW: http://jquense.github.io/react-widgets/docs/#/datetime-picker?_k=ymbdaq use the arrow keys to scroll up and down in the time list and you'll see what I mean.

@maps82
Copy link
Author

maps82 commented Aug 4, 2016

Hi. I'm still not exactly sure what you mean, but I can keyboard navigate in my modified RW version exactly as in yours. It's just that I pass a new prop through DateTimePicker to TimeList, that preselects 17h (in this example). It's not a real preselection, as the value is still empty, it's just that 17h is presented as first option (although you can scroll up to earlier times). To achieve that, this patch is needed.

If I press arrow up/down, the focusItem will NOT stick at the top, the behavior is identical to your example.

Also I tested that the patched scrollTo function always pulls the selected item into the viewport (no matter if it's below or above)

ezgif-3860083262

@jquense
Copy link
Member

jquense commented Aug 4, 2016

I think we are miss understanding each other. The default behavior of scrollTo is to only pull the item into view to it's closest edge. your patch only pulls the Item to the top of the viewport. that is weird behavior when scroll ing up from below the item.

@maps82
Copy link
Author

maps82 commented Aug 4, 2016

Hm, but that's exactly what I need in my case. It always puts the wanted item at the top, regardless if it's below or above the viewport. As far as possible of course - if it's one of the last items of the list it's not possible. Is there anything I can do to make it more generic to get the pr accepted?

Do you see the use case for RW? (see gif above: the user selects a timespan - usually business hours)

@jquense
Copy link
Member

jquense commented Aug 4, 2016

you are only thinking of the one case for RW, which is when you open the popup from closed. the same logic doesn't make sense for keyboard navigation...I'm actually curious how you do it in the one case but not the other. both scrolling hen open and scrolling when pressing arrowKey up/down.

@maps82
Copy link
Author

maps82 commented Aug 4, 2016

I'm actually curious how you do it in the one case but not the other. both scrolling when open and scrolling when pressing arrowKey up/down.

You can see this here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants