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

create scroll listener on mount instead of exported singleton #688

Merged
merged 3 commits into from
Jan 23, 2017

Conversation

jhsu
Copy link
Collaborator

@jhsu jhsu commented Jan 18, 2017

the existing module exports an instance of element-resize-detector which tries to bind event listeners on document.body which may not be available at instantiation.

PR Checklist

  • Manually tested across supported browsers
    • Chrome
    • Firefox
    • Safari
    • IE11 (Win 7)
    • Edge (Win 10)
  • Unit tests written (common at minimum)
  • PR has one of the semver- labels
  • Two core team engineer approvals
  • [ ] One core team UX approval

@jhsu
Copy link
Collaborator Author

jhsu commented Jan 18, 2017

this creates a new instance which probably creates a new listener per instance of the resizer.

one possibility that I know react-waypoint uses is creates an event listener on the parent's dom node so that any sibling checks the parent for a listener before creating its own.

@codecov-io
Copy link

codecov-io commented Jan 18, 2017

Current coverage is 91.28% (diff: 100%)

Merging #688 into master will not change coverage

@@             master       #688   diff @@
==========================================
  Files           158        157     -1   
  Lines          2662       2662          
  Methods         264        264          
  Messages          0          0          
  Branches        734        734          
==========================================
  Hits           2430       2430          
  Misses          221        221          
  Partials         11         11          
Diff Coverage File Path
•••••••••• 100% src/components/Resizer/Resizer.jsx

Powered by Codecov. Last update ce644df...0606d30

@sodiumjoe
Copy link
Contributor

sodiumjoe commented Jan 18, 2017

hm, generally there are at most a few resizing elements, right? Seems ok to me. Would be cool to implement the deduping logic you're talking about tho

import _ from 'lodash';
import React from 'react';
import { common } from '../../util/generic-tests';
import { mount } from 'enzyme';
import Resizer from './Resizer';
import { elementResizeDetector } from './Resizer.util';
Copy link
Contributor

Choose a reason for hiding this comment

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

wasn't there some reason we had to do this in the first place @mute ?

Copy link
Contributor

Choose a reason for hiding this comment

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

We only pulled this out to a util to maintain the singleton while making testing a little easier.

If we're okay with it not being a singleton, we don't need it anymore.

Copy link
Contributor

@jondlm jondlm left a comment

Choose a reason for hiding this comment

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

Yeah, I don't think this should cause issues.

@jondlm jondlm merged commit 5741799 into appnexus:master Jan 23, 2017
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.

6 participants