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

AngularJS scroller="" in directive not working. #147

Closed
digitaltopo opened this issue Feb 3, 2015 · 12 comments
Closed

AngularJS scroller="" in directive not working. #147

digitaltopo opened this issue Feb 3, 2015 · 12 comments

Comments

@digitaltopo
Copy link

I have an absolutely positioned div with an overflow-y:auto on it. I'd like to add headroom to an element within this div. I can't get the scroller="#scrollingcontainer" to work, instead I get this error:

Error: [jqLite:nosel] Looking up elements via selectors is not supported by jqLite!

I have included headroom.js (0.7.0) via bower, and added the dependency:
<script src="bower_components/headroom.js/dist/headroom.js"></script>
<script src="bower_components/headroom.js/dist/angular.headroom.js"></script>

angular
.module('app', [
'headroom',...

In my template I've included the directive:

...

I've tried these too:
scroller="#scrollingcontainer"
scroller=".scrollingcontainer" (for class)
for the scrolling container also.

What am I doing wrong??? It looks pretty clear in the documentation that you need to enter a selector that the directive then uses to get a dom element...

Any help would be appreciated, thanks!

@WickyNilliams
Copy link
Owner

Could you create a reduced test case?

@zwacky
Copy link
Contributor

zwacky commented Jul 15, 2015

if you're using jqLite only, without jQuery loaded beforehand. you can't look up by selector.

@zwacky
Copy link
Contributor

zwacky commented Jul 15, 2015

PR: #171

@reco
Copy link

reco commented Oct 29, 2015

+1

@ShaneZhengNZ
Copy link

@digitaltopo @WickyNilliams @zwacky You can't use the selector, instead you need to include the html string.

such as

<headroom tolerance='0' offset='0' scroller="<div class='app-view'></div>" classes="{pinned:'headroom--pinned',unpinned:'headroom--unpinned',initial:'headroom'}"></headroom>

@zwacky
Copy link
Contributor

zwacky commented Feb 10, 2016

why not just use one api as described in the docs (https://github.com/WickyNilliams/headroom.js#with-angularjs). is there a reason why you don't want to use document.querySelector?

@ShaneZhengNZ
Copy link

@zwacky
A few things:

  1. I agree with you. document.querySelector is faster, but document.getElementByid is the fastest. I would say if we want to change it, change to getElementById, and enforce the use to pass the id of the element. Best performance.
  2. As a workaround for now, I have to use the code I pasted above.
  3. Whether it is a good practice to use document.querySelector or document.getElementById in AngularJS? I am not sure.

@zwacky
Copy link
Contributor

zwacky commented Feb 10, 2016

you would cut away a lot from the versatility if it would need to be used with getElementById.

document.querySelector is perfectly fine to lookup elements and is very much supported (http://caniuse.com/#search=querySelector). I wouldn't force people to use IDs in their html just for this premature optimization - which can be certainly ignored performance-wise.

nowadays we are more aware of the webapp size, so fewer are running angular with jquery.

@ShaneZhengNZ
Copy link

@zwacky Sorry, but the workaround I said before did not work. It dismissed the console error, but the actual functionality is not working, because Angular.element does not select, instead it creates a copy.
Since your Pull request has been there for ever, and nobody merge it, I have to fork this, apply your fix to the fork and get my project to point to my fork. Painful.

@zwacky
Copy link
Contributor

zwacky commented Feb 12, 2016

i see. lemme update my pull request.

@zwacky
Copy link
Contributor

zwacky commented Feb 14, 2016

i just checked, there were no commits after this PR.

@WickyNilliams you see any problems with this?

@WickyNilliams
Copy link
Owner

Finally merged this. Sorry for the delay all :)

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

No branches or pull requests

5 participants