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

First Angular2 Prototype #251

Closed
wants to merge 4 commits into from
Closed

First Angular2 Prototype #251

wants to merge 4 commits into from

Conversation

sphaso
Copy link

@sphaso sphaso commented Feb 11, 2016

First prototype for an Angular2 version of the directive.
I've only manually tested the most basic case.

Problems and head-scratchers:

  1. not sure how to handle "THROTTLE_MILLISECONDS", need to read more docs on ng2 as they become available
  2. not sure how to handle "infiniteScrollListenForEvent" in a clean manner. I need to read more about how ng2 handles events and scoping
  3. why do you need scope.$eval on infiniteScrollImmediateCheck?
  4. the rationale behind the $watch-ers is that the bound value could change? (e.g. infiniteScrollDistance={someVarInControllerScope} ?)
  5. there was a variable "context" in func throttle which wasn't used anywhere, hopefully I didn't miss anything
  6. ng2 doesn't have an $interval service. My implementation of $interval(callback, delay, count) is a bit naive. I'd like to check the actual ng1 code
  7. I'm not sure why you needed "angular.element"
  8. Should the first PR take care of protractor \ tests \ build or can we think about it later once the codebase stabilizes?

@sphaso sphaso mentioned this pull request Feb 11, 2016
@sroze
Copy link
Owner

sroze commented Feb 11, 2016

Thank you for the effort you put on it @sphaso. Here are my answers:

  1. You can do without it at the moment.
  2. TBH, I don't remember
  3. Yes, this is the idea
  4. Yes, you can remove it.
  5. This is to remove the dependency to jQuery
  6. Definitely in my point of view. You can even remove the tests that are related to features you are not porting at the moment, but we need passing tests for all the claimed features :)

@sphaso
Copy link
Author

sphaso commented Feb 27, 2016

@sroze sorry this is taking a bit long. My main dev machine was in bad shape and had to borrow another pc to run the tests. I have 33 on 33 green specs. I hope they're not false positives :)

@sphaso
Copy link
Author

sphaso commented Mar 7, 2016

@sroze feel free to let me know if there's anything else you'd like me to do regarding this PR

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