-
Notifications
You must be signed in to change notification settings - Fork 76
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
Stop setting container scrollTop to zero on render if collection has offset #164
base: master
Are you sure you want to change the base?
Conversation
…l container, offset by some amount, and when rendering it, it should not reset the existing container scroll position
…ction offset is not zero
Ok, I will be out for the next two weeks but will take a look at it as soon as I’m back. Thank you! |
Hi, do you need anything else for the PR to be merged ? |
I don't think this is quite the right fix unfortunately, and I haven't had the time to look at the issue and fix it myself. If you can rebase and dive in a bit deeper to try to get the right fix I can try to get it merged asap, I'm also available for guidance if needed.
This PR would disable all of that logic if the VC had an offset in its container. It's surprising to me that no tests fail, definitely points to a lack of coverage in this area. The correct fix would account for Feel free to reach out in the #e-smoke-and-mirrors on the Ember Community slack, or on here! |
I originally setup vertical-collection to allow for arbitrary padding on either side of the scrollable content in the scroll container to support loading states, table headers/footers and the like. It also helps for when the document body is the scroll container. Seems bad if this is broken :D I read through the comments on the other thread and peeked at this. I agree this solution here isn't quite right, but if it gets rebased I'm sure we can massage it to a good place. |
Add scroll test where a collection that is in the middle of the scroll container, offset by some amount, and when rendering it, it should not reset the existing container scroll position.
See comment