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

Add scrollTo helper #698

Merged
merged 1 commit into from
Apr 20, 2020
Merged

Add scrollTo helper #698

merged 1 commit into from
Apr 20, 2020

Conversation

nlfurniss
Copy link
Contributor

Inspired by the scrollTo helper from ember-native-dom-helpers.

If people are receptive to this, I'll add some tests :)

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good generally, tests and some more docs (thinking some examples) seem like a good next step...

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updates look good, I think we need a few more tests for the various error conditions that we throw for.

tests/integration/dom/scroll-to-test.js Outdated Show resolved Hide resolved
tests/integration/dom/scroll-to-test.js Outdated Show resolved Hide resolved
addon-test-support/@ember/test-helpers/dom/scroll-to.ts Outdated Show resolved Hide resolved
@nlfurniss nlfurniss force-pushed the add-scroll-to branch 2 times, most recently from 18b0e76 to 182dc05 Compare August 30, 2019 21:10
@rwjblue
Copy link
Member

rwjblue commented Aug 31, 2019

I think I've gotten CI fixed on master now, mind rebasing?

@nlfurniss
Copy link
Contributor Author

Investigating why this is failing in Ember 2.0 for Cannot read property 'buildInstance' of undefined

@cibernox
Copy link
Contributor

I'd love to see this ported!

@nlfurniss
Copy link
Contributor Author

Removed scrolling of window/document, added tests for error states

@rwjblue
Copy link
Member

rwjblue commented Sep 23, 2019

Looks like a downstream package has dropped support for Node 6 without bumping major versions (therefore breaking CI for us when using "floating dependencies"). I reported the issue over in socketio/engine.io#589, but I'm not sure yet if they will fix the issue.

If it does not look like they intend to resolve that issue in a day or so, I'll mark the floating dependencies CI run as an allowed failure so that we are unblocked.

@nlfurniss
Copy link
Contributor Author

@rwjblue seems like this issue in socketio isn't getting fixed, would it be possible to allow this failure?

@rwjblue
Copy link
Member

rwjblue commented Apr 20, 2020

@nlfurniss - We have dropped support for Node 8 and lower now, I think this will pass with a rebase...

@rwjblue rwjblue changed the title Add scroll-to helper Add scrollTo helper Apr 20, 2020
@nlfurniss nlfurniss force-pushed the add-scroll-to branch 2 times, most recently from aae1605 to e5652c7 Compare April 20, 2020 19:02
@rwjblue
Copy link
Member

rwjblue commented Apr 20, 2020

Rerolled yarn.lock in #814 and restarted the build here to try to fix those TS issues.

@nlfurniss
Copy link
Contributor Author

passes 🎉

@rwjblue rwjblue merged commit 8548fd3 into emberjs:master Apr 20, 2020
@rwjblue
Copy link
Member

rwjblue commented Apr 20, 2020

Thank you @nlfurniss!

@nlfurniss nlfurniss deleted the add-scroll-to branch April 20, 2020 22:38
@amk221
Copy link
Contributor

amk221 commented Jun 11, 2020

Is this missing from API.md?

@nlfurniss
Copy link
Contributor Author

@amk221 yes it is, I'll create an issue to add it once this code is published in a non-beta version

@amk221
Copy link
Contributor

amk221 commented Jun 11, 2020

ah ok, just spotted randomly. 👍

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

Successfully merging this pull request may close these issues.

4 participants