Ignores top alignment if top offset not given

Question

Ignores top alignment if top offset not given

Closed this issue 8 years ago · 9 comments

Finally a real bug.

These lines are problematic:

leftOffset = align && align.leftOffset !== null ? align.leftOffset : 0,
topOffset = align && align.topOffset !== null ? align.topOffset : 0,

If you specify alignment options only containing a top value, it will not work (doesn't scroll at all). Only after you add topOffset: 0 explicitly does it work again.

I suggest the above lines are altered to something like:

leftOffset = align && typeof align.leftOffset === 'number' ? align.leftOffset : 0,
topOffset = align && typeof align.topOffset === 'number' ? align.topOffset : 0,

That way they will default to 0 if the option is not provided.

Answer 1 · 2017-05-19T07:00:56.000Z

That's no different as long as you don't pass non-numbers to the call. What's the configuration you are using?

Answer 2 · 2017-05-19T07:02:52.000Z

Here it is working in the example: https://github.com/KoryNunn/scroll-into-view/blob/master/example/index.js#L29

Answer 3 · 2017-05-19T20:08:07.000Z

I am implicitly passing undefined, because I am not defining the property, and the original code then leaves this as undefined because undefined does not equal null. This then stops the scroll from working, unless I explicitly pass 0.

Maybe the examples use simplistic non-real world HTML, I don't know. Also, my problem was with top: 1 not top: 0. That could affect it as well.

This is the full config passed:

        time: 250,
        align: {
          top: 1,
        },
        validTarget(target, parentsScrolled) {
          return (
            parentsScrolled < 2 && target !== window &&
            !target.matches('.modal')
          );
        },

Answer 4 · 2017-05-20T00:27:32.000Z

The code in the source is

leftOffset = align && align.leftOffset != null ? align.leftOffset : 0,
        topOffset = align && align.topOffset != null ? align.topOffset : 0,

Are you installing this from NPM? Which version?

Answer 5 · 2017-05-20T01:49:12.000Z

I copied your code from the github repo, because I can't run the package in the browser due to the way that you've included require('raf') and my build process doesn't cater for requires, so I get a require is undefined error.

It seems after copying, my linter fixed your != comparison to !== because that's best practice for javascript. (E.g. null !== undefined). Probably in your version it doesn't care due to the !=.

Answer 6 · 2017-05-20T08:08:31.000Z

Ok so this is an issue with your build process.

!= null is a perfectly reasonable way to check something is neither undefined or null.

I'd suggest you build this module with browserify and then use the built file as a standalone.

Answer 7 · 2017-05-20T20:43:44.000Z

I'm sorry, but I think you're being somewhat short sighted -- my build process is perfectly fine, and it's been used in many commercial projects. it's the way you package this repository is problematic, because it can't be consumed by different build processes. Not everyone uses browserify, nor wants to use browserify, so you can't force it on people. The right way to go about it is to pre-build a standalone JS file that can be included by any build process/browser and will work right off the bat.

Consumers of this script don't care much how you built your package, what build tools they need to get it up and running, what the dependencies are, or whether or not it's written in TypeScript or ES6 or ES3. They just want to include it and run it, and you as package developer should provide that option or you will alienate a large user base.

Answer 8 · 2017-05-20T23:08:28.000Z

Pull requests welcome.

Answer 9 · 2017-06-22T01:39:52.000Z

@adamreisnz I created a minified version after a polite request in issue #32