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

Fix for items larger than viewport #62

Closed
wants to merge 9 commits into from

Conversation

rhengeveld
Copy link

For each axis (X,Y) the item is considered in the viewport when:

  • It's dimensions are fully within the viewport (original calculation)
  • It's dimension are fully outside the viewport (new)

So the behaviour only changes for items that are actually taller/wider :)

This probably also fixes #37

For each axis (X,Y) the item is considered in the viewport when:  
- It's dimensions are fully within the viewport (original calculation)
- It's dimension are fully outside the viewport (new)

So the behaviour only changes for items that are actually taller/wider :)

This probably also fixes DockYard#37
@rhengeveld
Copy link
Author

Sadly I don't know much about automated testing yet @ Javascript, so I don't know what's with the Travis CI build failing 😞

@awaer
Copy link

awaer commented Apr 12, 2016

@rhengeveld The Travis CI failure isn't related to your PR; it looks like a recent update to the ember-suave addon (#59) is now causing a linter error due to the new requireSpread JSCS rule. This is also mentioned in DockYard/ember-suave#109, in which @rwjblue suggests proposing a PR to fix this upstream.

For reference, here is the output from Travis CI:

not ok 10 Chrome 49.0 - JSCS - helpers: helpers/module-for-acceptance.js should pass jscs
    ---
        actual: >
            false
        expected: >
            true
        stack: >
                at http://localhost:7357/assets/test-support.js:4039:12
                at Object.<anonymous> (http://localhost:7357/assets/tests.js:127:5)
                at runTest (http://localhost:7357/assets/test-support.js:2688:28)
                at Object.Test.run (http://localhost:7357/assets/test-support.js:2673:4)
                at http://localhost:7357/assets/test-support.js:2815:11
                at process (http://localhost:7357/assets/test-support.js:2474:24)
        message: >
            helpers/module-for-acceptance.js should pass jscs.
            requireSpread: Illegal use of apply method. Use the spread operator instead at helpers/module-for-acceptance.js :
                 9 |
                10 |      if (options.beforeEach) {
                11 |        options.beforeEach.apply(this, arguments);
            -----------------------------------^
                12 |      }
                13 |    },
            requireSpread: Illegal use of apply method. Use the spread operator instead at helpers/module-for-acceptance.js :
                17 |
                18 |      if (options.afterEach) {
                19 |        options.afterEach.apply(this, arguments);
            ----------------------------------^
                20 |      }
                21 |    }
   ...

@poteto
Copy link
Collaborator

poteto commented Apr 22, 2016

@rhengeveld Thanks for this PR! If you rebase against the latest master, your build should stop breaking.

@rhengeveld
Copy link
Author

@poteto Thanks for the heads up, this seems to do the trick! 😄

@markmhendrickson
Copy link

It'd be great to have this PR merged. I'm also dealing with this problem of elements taller than the viewport not triggering hooks.

@jasonmit
Copy link

Something to consider, if not already considered, window.innerWidth and window.innerHeight can trigger a reflow.

@homu
Copy link
Contributor

homu commented Jul 27, 2016

☔ The latest upstream changes (presumably 082e884) made this pull request unmergeable. Please resolve the merge conflicts.

@homu
Copy link
Contributor

homu commented Jan 7, 2017

☔ The latest upstream changes (presumably e70de8f) made this pull request unmergeable. Please resolve the merge conflicts.

@snewcomer
Copy link
Collaborator

If using the latest releases of this addon, IntersectionObserver will always trigger once and check didEnterViewport. Closing for now, but feel free to open again if need be!

@snewcomer snewcomer closed this Apr 15, 2018
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.

Elements taller than viewport never trigger hooks
9 participants