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 option to specify scrollable element #2

Merged
merged 3 commits into from
Mar 25, 2015

Conversation

greis
Copy link
Contributor

@greis greis commented Mar 24, 2015

The scrollable defaults to window if the option is not used.
Usage:

<div id="content">
{{#each model as |product|}}
  <h1>{{product.name}}</h1>
  <h2>{{product.description}}</h1>
{{/each}}
</div>
{{infinity-loader infinityModel=model scrollable="#content"}}

@hhff
Copy link
Collaborator

hhff commented Mar 24, 2015

Thanks @greis! This is awesome and definitely needed - however my concern here is that we're relying on jQuery to figure out where on the page scrollable actually is. If there's more than one DOM element matching the scrollable selector string, it may be confusing for the developer.

We need to safeguard this!

My suggestion is that we:

  • make scrollable a single DOM node object and validate this in the component, or
  • or on "didInsertElement" we cache the selector on the component as a $(DOM node) - and if jQuery happens to return an array that is larger in length than 1, we throw an error.

We can probably do both by checking if scrollable is an instanceof a Object or String.

@greis
Copy link
Contributor Author

greis commented Mar 24, 2015

Just pushed some changes to cover the suggestions you made. Also wrote some unit tests for it. What do you think?

@hhff
Copy link
Collaborator

hhff commented Mar 24, 2015

Yes!! This is awesome - and thankyou for writing tests! I've got a couple lil' tweaks - but we're pretty much there:

  • can we call _checkScrollable first?
  • Scrollable starts as a "non-jqueryified" window. Let's have it start as null and instead have it fall back to $(window) if the user doesn't pass in a selector string. This way we're certain "scrollable" is always a jquery element regardless of user input!
  • this also means whenever we refer to this.get('scrollable') it won't need to be wrapped in Ember.$()
  • Given the above perhaps _setupScrollable is more semantic
  • Nitpick, but in line 41 you're calling this.get('scrollable') twice. Can we cache this in a var to avoid doubling the getter?
  • Would be great to add this to the README too!

Thanks so much here @greis !

@greis
Copy link
Contributor Author

greis commented Mar 25, 2015

Done. I will leave it to you to update the README :)

hhff added a commit that referenced this pull request Mar 25, 2015
Add option to specify scrollable element
@hhff hhff merged commit e0ae3fc into adopted-ember-addons:master Mar 25, 2015
@hhff
Copy link
Collaborator

hhff commented Mar 25, 2015

TY!

mansona pushed a commit that referenced this pull request May 15, 2024
@github-actions github-actions bot mentioned this pull request May 16, 2024
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.

3 participants