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

check if in view on resize events #45

Merged
merged 1 commit into from Jun 22, 2015
Merged

check if in view on resize events #45

merged 1 commit into from Jun 22, 2015

Conversation

kellyselden
Copy link
Collaborator

If you maximize a small window and the loader comes into view, it will looks like it hangs. This will watch the resize event and check if in view. Paired with #44, maximizing a window will load as many times as necessary to fill up the page.

var scrollable = this.get('scrollable');
if (Ember.typeOf(scrollable) === 'string') {
var items = Ember.$(scrollable);
_setupEvent(type) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpick - but technically we're searching / setting up the bindable dom element for the event, rather than the event itself.

Maybe we should call this _setupBindable or something instead?

@hhff
Copy link
Collaborator

hhff commented Jun 21, 2015

1 comment - but also needs a master merge before I can merge. keen to cut a new release when this goes in!

thanks for your great work @kellyselden !

@kellyselden
Copy link
Collaborator Author

No problem @hhff.

I also merged _bindScroll and _bindResize into _bindEvent to cut down on duplication.

Line 46 has got me thinking. Perhaps the scrollable and resizeable properties should be only one property/selector to cover both scenarios. Does it ever make sense for the elements to be different?

@kellyselden
Copy link
Collaborator Author

I wonder if it would be better to leave out the resizable property and use the scrollable selector for the resizable logic, if in fact it doesn't make sense for them to be different.

@hhff
Copy link
Collaborator

hhff commented Jun 22, 2015

I agree @kellyselden - maybe lets call it bindable? (we'll also need to fallback to scrollable for backward compatibility)

@kellyselden
Copy link
Collaborator Author

Would it be so bad to still call it scrollable and just hide the fact we are watching resize as well? or does that leave a bad taste?

@hhff
Copy link
Collaborator

hhff commented Jun 22, 2015

@kellyselden nah das cool with me!

@kellyselden
Copy link
Collaborator Author

should be good now, feels a little cleaner

hhff added a commit that referenced this pull request Jun 22, 2015
@hhff hhff merged commit 1e556e3 into adopted-ember-addons:master Jun 22, 2015
@hhff
Copy link
Collaborator

hhff commented Jun 22, 2015

way way nicer @kellyselden - ur the man.

@hhff
Copy link
Collaborator

hhff commented Jun 22, 2015

gonna cut a release shortly

@kellyselden kellyselden deleted the handle-resize-event branch June 22, 2015 20:47
@hhff
Copy link
Collaborator

hhff commented Jun 22, 2015

@kellyselden 0.0.10 is up on NPM

@kellyselden
Copy link
Collaborator Author

@hhff awesome. Thanks.

@hhff
Copy link
Collaborator

hhff commented Jun 22, 2015

No thankyou dude!!!
On Mon, 22 Jun 2015 at 5:13 pm Kelly Selden notifications@github.com
wrote:

@hhff https://github.com/hhff awesome. Thanks.


Reply to this email directly or view it on GitHub
#45 (comment).

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

2 participants