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

fixes distance from scroll #41

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

Conversation

jakeleboeuf
Copy link

This PR uses @caltabid's suggested fix to address issue #38.

@sardyy
Copy link

sardyy commented May 14, 2018

@Duder-onomy Hello! Can you check this pull request and launch a new release? It fixes some problems I'm facing right now.

@Duder-onomy
Copy link
Contributor

Duder-onomy commented May 14, 2018

I just took a look and this seems reasonable to me, anything that can be done to simplify the get the elements top logic is good. Considering people claim that this is fixing something, can you manage to write a failing test? If not, why do the current tests pass when they are broken in your implementation? The app I work on uses this allot, this page for example uses the lib to power those ['overview', 'details', 'location', 'reviews'] tabs, they work fine when scrolling in reverse (which I believe was the original issue in #38)

Anyways, I just pulled this in and ran the test suite on the decently massive ember app I work on and things look good.

However, @sardyy I cannot merge things here. I don't have those permissions.

------------ UPDATE -------------

I can confirm that 0.6.4 is broken, but 0.6.5 work fine for me, #37 fixed this issue for me, but we need to get 0.6.5 out.
@ragnarpeterson #40 needs to happen. Then I think this PR can be closed.

@sardyy
Copy link

sardyy commented May 15, 2018

Hey @Duder-onomy.

First of all, thank you for your reply.

Yeah, you are right. I wrote this comment before testing the addon using the master branch and it's actually working, so this PR doesn't fix anything related to my problem because it was already fixed with 0.6.5.

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.

None yet

3 participants