ScrollFloat: allow a content context used to measure distance from bottom of content vs. scroll context. #165

Merged
merged 2 commits into from Jan 11, 2017

Projects

None yet

4 participants

@mikesherov
Contributor

No description provided.

@nemtsov

Nice!

if (typeof breakpoint === 'function') {
+ contentContext = context;
context = callback;
callback = breakpoint;
breakpoint = 1;
}
context = context || 'window';
breakpoint = Number(breakpoint).toString();
@nemtsov
nemtsov Jan 10, 2017 Member

I know you didn't update this piece, but according to the comment, a breakpoint could be 50%. Number('50%').toString() returns "NaN".

@mikesherov
mikesherov Jan 10, 2017 Contributor

yeah, I don't know why it does this... it should just be a Number to begin with... Don't feel like playing Jenga right now though.

@mikesherov
mikesherov Jan 11, 2017 Contributor

addressed the comment, because 50% would be totally broken.

@nemtsov
Member
nemtsov commented Jan 10, 2017

I think it would be great to start extracting libraries such as this one into beff-independent libraries, until beff is left with nothing.

@eerikson

Hey, you did a great job cleaning this up. I really only found one thing that jumped out at me. Not a blocker though I suppose.

// search for the original function
i = cb.original.indexOf(fn);
if (~i) {
cb.original.splice(i, 1);
cb.wrapped.splice(i, 1);
if (!cb.original.length) {
- delete registry[context][breakpoint];
+ delete registry[contextId][breakpoint];
@eerikson
eerikson Jan 10, 2017

I know you didn't really come up with the idea for delete but could this be upgraded to splice instead?

@mikesherov
mikesherov Jan 10, 2017 Contributor

registry[contextId][breakpoint] is an object, not an array.

@eerikson
eerikson Jan 11, 2017 edited

Ah! Didn't realize this wasn't an index. Using it for objects is much safer, but I'd still warn against it for performance reasons you can see here.

@mikesherov
mikesherov Jan 11, 2017 Contributor

It's not only a safety thing, it's a correctness thing. delete will change the length of Object.keys, which is the goal.

Furthermore, this is not inside a scroll event, and is intended to be run once. Microbenchmarks on jsperf (BTW, I used to contribute code to that site!) is definitely a premature optimization here.

@eerikson
eerikson Jan 11, 2017

Very cool and totally fair!

- delete registry[context];
+ if (!Object.keys(registry[contextId]).length) {
+ $context.off('scroll', scrollCache[contextId]);
+ delete registry[contextId];
@eerikson
eerikson Jan 10, 2017

See above re: delete vs splice

@mikesherov
mikesherov Jan 10, 2017 Contributor

registry[contextId] is an object, not an array.

@eerikson
eerikson Jan 11, 2017 edited

See above! Less of a big deal with objects.

@mikesherov
mikesherov Jan 11, 2017 Contributor

See the above reply!

@eerikson

👍

@agaripian
Contributor

👍

@mikesherov mikesherov merged commit cdfa6d7 into behance:master Jan 11, 2017

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@mikesherov mikesherov deleted the mikesherov:scrollFloat branch Jan 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment