-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Add custom element option to WindowScroller #481
Changes from 23 commits
b7ac151
754cb2a
1981162
b9399dd
a0df5f8
57efa5f
27f2b69
e06f19e
ca60f00
8da79fb
6fd41a0
18612a6
8ff9cfc
dc7a7ac
2a90fbf
36e14b6
a8f039f
42493ce
2ddaed2
919e342
197a5c3
c7bb0b7
cc83019
643d8a2
05570f8
2a721a7
fe11056
78781c7
335d6df
15c9fde
bb804f2
29cec61
5c2b141
d44dde1
188d691
43691da
c86015e
2e91ec7
32454e0
36a54f9
87fe0f1
f2199ac
7c78188
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ import { Component, PropTypes } from 'react' | |
import ReactDOM from 'react-dom' | ||
import shallowCompare from 'react-addons-shallow-compare' | ||
import { registerScrollListener, unregisterScrollListener } from './utils/onScroll' | ||
import { getVerticalScroll, getPositionFromTop, getHeight } from './utils/dimensions' | ||
|
||
export default class WindowScroller extends Component { | ||
static propTypes = { | ||
|
@@ -13,6 +14,9 @@ export default class WindowScroller extends Component { | |
*/ | ||
children: PropTypes.func.isRequired, | ||
|
||
/** Element to attach scroll event listeners. Defaults to window. */ | ||
scrollElement: PropTypes.any, | ||
|
||
/** Callback to be invoked on-resize: ({ height }) */ | ||
onResize: PropTypes.func.isRequired, | ||
|
||
|
@@ -28,8 +32,8 @@ export default class WindowScroller extends Component { | |
constructor (props) { | ||
super(props) | ||
|
||
const height = typeof window !== 'undefined' | ||
? window.innerHeight | ||
const height = typeof this.scrollElement !== 'undefined' | ||
? getHeight(this.scrollElement) | ||
: 0 | ||
|
||
this.state = { | ||
|
@@ -43,27 +47,31 @@ export default class WindowScroller extends Component { | |
this._enablePointerEventsAfterDelayCallback = this._enablePointerEventsAfterDelayCallback.bind(this) | ||
} | ||
|
||
componentDidMount () { | ||
const { height } = this.state | ||
// Can’t really use defaultProps for `window` without breaking server-side rendering | ||
get scrollElement () { | ||
return this.props.scrollElement || window | ||
} | ||
|
||
// Subtract documentElement top to handle edge-case where a user is navigating back (history) from an already-scrolled bage. | ||
// In this case the body's top position will be a negative number and this element's top will be increased (by that amount). | ||
this._positionFromTop = | ||
ReactDOM.findDOMNode(this).getBoundingClientRect().top - | ||
document.documentElement.getBoundingClientRect().top | ||
componentDidMount () { | ||
this._updateDimensions() | ||
registerScrollListener(this, this.scrollElement) | ||
window.addEventListener('resize', this._onResizeWindow, false) | ||
} | ||
|
||
if (height !== window.innerHeight) { | ||
this.setState({ | ||
height: window.innerHeight | ||
}) | ||
componentWillReceiveProps (nextProps) { | ||
if (nextProps.scrollElement && nextProps.scrollElement !== this.scrollElement) { | ||
this._updateDimensions(nextProps.scrollElement) | ||
unregisterScrollListener(this, this.scrollElement) | ||
registerScrollListener(this, nextProps.scrollElement) | ||
} else if (!nextProps.scrollElement && this.scrollElement !== window) { | ||
this._updateDimensions(window) | ||
unregisterScrollListener(this, this.scrollElement) | ||
registerScrollListener(this, window) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Am I reading this wrong, or could we reduce the complexity of this a bit? if (nextProps.scrollElement !== this.scrollElement) {
this._updateDimensions(nextProps.scrollElement || window)
unregisterScrollListener(this, this.scrollElement)
registerScrollListener(this, nextProps.scrollElement || window)
} |
||
} | ||
|
||
registerScrollListener(this) | ||
window.addEventListener('resize', this._onResizeWindow, false) | ||
} | ||
|
||
componentWillUnmount () { | ||
unregisterScrollListener(this) | ||
unregisterScrollListener(this, this.scrollElement) | ||
|
||
window.removeEventListener('resize', this._onResizeWindow, false) | ||
} | ||
|
@@ -83,6 +91,19 @@ export default class WindowScroller extends Component { | |
return shallowCompare(this, nextProps, nextState) | ||
} | ||
|
||
_updateDimensions (scrollElement = this.scrollElement) { | ||
const { height } = this.state | ||
|
||
this._positionFromTop = getPositionFromTop(ReactDOM.findDOMNode(this), scrollElement) | ||
|
||
const newHeight = getHeight(scrollElement) | ||
if (height !== newHeight) { | ||
this.setState({ | ||
height: newHeight | ||
}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is probably the place to call the |
||
} | ||
} | ||
|
||
_enablePointerEventsAfterDelayCallback () { | ||
this.setState({ | ||
isScrolling: false | ||
|
@@ -92,7 +113,7 @@ export default class WindowScroller extends Component { | |
_onResizeWindow (event) { | ||
const { onResize } = this.props | ||
|
||
const height = window.innerHeight || 0 | ||
const height = getHeight(this.scrollElement) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should probably call The _onResizeWindow (event) {
this._updateDimensions()
} |
||
|
||
this.setState({ height }) | ||
|
||
|
@@ -102,10 +123,7 @@ export default class WindowScroller extends Component { | |
_onScrollWindow (event) { | ||
const { onScroll } = this.props | ||
|
||
// In IE10+ scrollY is undefined, so we replace that with the latter | ||
const scrollY = ('scrollY' in window) | ||
? window.scrollY | ||
: document.documentElement.scrollTop | ||
const scrollY = getVerticalScroll(this.scrollElement) | ||
|
||
const scrollTop = Math.max(0, scrollY - this._positionFromTop) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
/** | ||
* Gets the vertical scroll amount of the element, accounting for IE compatibility | ||
* and API differences between `window` and other DOM elements. | ||
*/ | ||
export function getVerticalScroll (element) { | ||
return element === window | ||
? (('scrollY' in window) ? window.scrollY : document.documentElement.scrollTop) | ||
: element.scrollTop | ||
} | ||
|
||
/** | ||
* Gets the height of the element, accounting for API differences between | ||
* `window` and other DOM elements. | ||
*/ | ||
export function getHeight (element) { | ||
return element === window | ||
? window.innerHeight | ||
: element.getBoundingClientRect().height | ||
} | ||
|
||
/** | ||
* Gets the vertical position of an element within its scroll container. | ||
* Elements that have been “scrolled past” return negative values. | ||
* Handles edge-case where a user is navigating back (history) from an already-scrolled page. | ||
* In this case the body’s top position will be a negative number and this element’s top will be increased (by that amount). | ||
*/ | ||
export function getPositionFromTop (element, container) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was having trouble wrapping my mind around this (previously here), so extra care in making sure this satisfies the original intention is appreciated. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay. When I do a real review, I'll read this one carefully. |
||
const containerElement = container === window ? document.documentElement : container | ||
return element.getBoundingClientRect().top - containerElement.getBoundingClientRect().top | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
.ContentBox { | ||
flex: 1; | ||
flex: 1 0 auto; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is obviously not directly related to the source changes, but something to this effect was required to make my addition to the demo work (a ContentBox was shrinking and scrolling, rather than There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No strong feelings here. That's fine. 😄 |
||
display: flex; | ||
flex-direction: column; | ||
background-color: #FFF; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is probably necessary to test the change within the demo site, but I'm not a fan of the complexity this adds to the
WindowScroller.example
orApplication
components. I wonder if it wouldn't be better to leave this particular functionality out of the demo and just cover it via unit tests instead?Speaking of which 😁 this change set should be accompanied by a couple of new unit tests so others (including myself) don't break it in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally. I was hoping to get your thoughts on what/how to test. Was wondering if some of the existing WindowScroller tests could be extracted into a function and be called once for each "mode" (scrolling
window
and scrollingscrollElement
). Does that make sense or should each new test be written out distinctly? Do you want to duplicate some of the basic assertions to make sure WindowScroller as a whole doesn’t mess up withscrollElement
, or should the new tests only address the specific added functionality?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One way I test things like this is by iterating over a set of options and creating my tests (the
describe
block) inside of a loop. egRather than extracting tests into helper functions. I'm not convinced that's the appropriate way to handle this case though. But new functionality like this absolutely needs new test coverage. 😄