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

Add custom element option to WindowScroller #481

Closed
wants to merge 43 commits into from
Closed
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
b7ac151
Add util for abstracting API differences between window and other con…
andrewbranch Nov 22, 2016
754cb2a
Modify onScroll utils to accept arbitrary scroll element
andrewbranch Nov 22, 2016
1981162
Allow WindowScroller to receive custom scrollElement prop
andrewbranch Nov 22, 2016
b9399dd
Fix copy/paste error
andrewbranch Nov 22, 2016
a0df5f8
Look at what element scroll listener was attached to, not initiator
andrewbranch Nov 22, 2016
57efa5f
Update demo to include custom scroll element option
andrewbranch Nov 22, 2016
27f2b69
Fix case where prop changes from custom element to nothing
andrewbranch Nov 22, 2016
e06f19e
Make sure my custom element is scrolling, not ContentBox
andrewbranch Nov 22, 2016
ca60f00
Auto fix linter errors
andrewbranch Nov 22, 2016
8da79fb
Fix linter errors / things that will break server-side
andrewbranch Nov 22, 2016
6fd41a0
Update pointerEvents assertions
andrewbranch Nov 22, 2016
18612a6
Fix failing "restore pointer events on unmount" test
andrewbranch Nov 22, 2016
8ff9cfc
Add missing _updateDimensions call
andrewbranch Nov 22, 2016
dc7a7ac
Replace PropTypes.object with any for HTMLElement props
andrewbranch Nov 22, 2016
2a90fbf
Add autoprefixer-loader (for now?)
andrewbranch Nov 22, 2016
36e14b6
Prevent internal scroll position overwriting scroll position from props
Nov 23, 2016
a8f039f
Updated cellRenderer callback parameters to also match List rowRenderer
romulof Nov 29, 2016
42493ce
Update CellMeasurer documentation
romulof Nov 29, 2016
2ddaed2
Fixed code-style issue
romulof Nov 29, 2016
919e342
Updated tests for new CellMeasurer render :index param
Nov 30, 2016
197a5c3
Removed outdated warning about using CellMeasurer with List now that …
Nov 30, 2016
c7bb0b7
Updated CHANGELOG in advance of upcoming release
Nov 30, 2016
cc83019
Merge pull request #482 from lic-nz/fix-scroll-position-from-props
mbrevda Dec 1, 2016
643d8a2
Minor formatting tweaks
Dec 1, 2016
05570f8
Updated CHANGELOG for upcoming release
Dec 1, 2016
2a721a7
Prepping 8.6.0 release
Dec 1, 2016
fe11056
Revert "Add autoprefixer-loader (for now?)"
andrewbranch Dec 1, 2016
78781c7
Revert "Modify onScroll utils to accept arbitrary scroll element"
andrewbranch Dec 1, 2016
335d6df
onScroll take 2: attach listener to arbitrary scroll element, but onl…
andrewbranch Dec 1, 2016
15c9fde
Revert "Update pointerEvents assertions"
andrewbranch Dec 1, 2016
bb804f2
Fix clearTimeout never being called
andrewbranch Dec 1, 2016
29cec61
Update CellSizeCache interface for better perfomance (#486)
arusakov Dec 1, 2016
5c2b141
Merge branch 'arusakov-486_cell_size_cache_optimisation'
Dec 2, 2016
d44dde1
Prepping for 8.6.1 release
Dec 2, 2016
188d691
Fix checkbox bug/typo
andrewbranch Dec 2, 2016
43691da
Fix demo bug: make sure container does not shrink when Grid is not ye…
andrewbranch Dec 2, 2016
c86015e
Fix bug where new height was calculated with old element (never changed)
andrewbranch Dec 2, 2016
2e91ec7
Added to to handle case when header items change or resize. also b…
Dec 3, 2016
32454e0
Reverted part of the change introduced in version 8.6.0 that changed …
Dec 3, 2016
36a54f9
Resolved conflicts
Dec 3, 2016
87fe0f1
Fix positionFromTop calculation
andrewbranch Dec 15, 2016
f2199ac
Add scroll handlers to distinct scrollElements
andrewbranch Dec 22, 2016
7c78188
Avoid some unnecessary updates to some WindowScrollers
andrewbranch Jan 11, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 25 additions & 4 deletions source/WindowScroller/WindowScroller.example.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,19 +9,27 @@ import AutoSizer from '../AutoSizer'
import shallowCompare from 'react-addons-shallow-compare'
import styles from './WindowScroller.example.css'

export default class AutoSizerExample extends Component {
export default class WindowScrollerExample extends Component {
static contextTypes = {
list: PropTypes.instanceOf(Immutable.List).isRequired
list: PropTypes.instanceOf(Immutable.List).isRequired,
customElement: PropTypes.any,
isScrollingCustomElement: PropTypes.bool.isRequired,
setScrollingCustomElement: PropTypes.func
}

constructor (props) {
super(props)

this._rowRenderer = this._rowRenderer.bind(this)
this.onChangeCustomElementCheckbox = this.onChangeCustomElementCheckbox.bind(this)
}

onChangeCustomElementCheckbox (event) {
this.context.setScrollingCustomElement(event.target.checked)
}

render () {
const { list } = this.context
const { list, isScrollingCustomElement, customElement } = this.context
Copy link
Owner

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 or Application 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.

Copy link
Contributor Author

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 scrolling scrollElement). 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 with scrollElement, or should the new tests only address the specific added functionality?

Copy link
Owner

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. eg

for (let i = 0; i < 2; i++) {
  describe('WindowScroller', () => {
    const useCustomElement = !!i;
    // Tests here ...
  });
}

Rather 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. 😄


return (
<ContentBox>
Expand All @@ -36,8 +44,21 @@ export default class AutoSizerExample extends Component {
and manages the window scroll to scroll through the list
</ContentBoxParagraph>

<ContentBoxParagraph>
<label className={styles.checkboxLabel}>
<input
aria-label='Use custom element for scrolling'
className={styles.checkbox}
type='checkbox'
checked={isScrollingCustomElement}
onChange={this.onChangeCustomElementCheckbox}
/>
Use custom element for scrolling
</label>
</ContentBoxParagraph>

<div className={styles.WindowScrollerWrapper}>
<WindowScroller>
<WindowScroller scrollElement={isScrollingCustomElement ? customElement : null}>
{({ height, isScrolling, scrollTop }) => (
<AutoSizer disableHeight>
{({ width }) => (
Expand Down
62 changes: 40 additions & 22 deletions source/WindowScroller/WindowScroller.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand All @@ -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,

Expand All @@ -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 = {
Expand All @@ -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)
Copy link
Owner

Choose a reason for hiding this comment

The 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)
}
Expand All @@ -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
})
Copy link
Owner

Choose a reason for hiding this comment

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

This is probably the place to call the onResize prop too.

}
}

_enablePointerEventsAfterDelayCallback () {
this.setState({
isScrolling: false
Expand All @@ -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)
Copy link
Owner

Choose a reason for hiding this comment

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

We should probably call this._updateDimensions() here instead? That will handle cases where a resize affects top position (due to things like text wrapping) and it will also only call setState if the height has actually changed (eg not just a horizontal resize).

The onResize call should also be moved into this helper method for similar reasons (and to avoid duplication).

_onResizeWindow (event) {
  this._updateDimensions()
}


this.setState({ height })

Expand All @@ -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)

Expand Down
30 changes: 30 additions & 0 deletions source/WindowScroller/utils/dimensions.js
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) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Owner

Choose a reason for hiding this comment

The 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
}
13 changes: 6 additions & 7 deletions source/WindowScroller/utils/onScroll.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,27 +35,26 @@ function enablePointerEventsAfterDelay () {
}

function onScrollWindow (event) {
if (originalBodyPointerEvents == null) {
if (event.currentTarget === window && originalBodyPointerEvents == null) {
originalBodyPointerEvents = document.body.style.pointerEvents

document.body.style.pointerEvents = 'none'

enablePointerEventsAfterDelay()
}
enablePointerEventsAfterDelay()
mountedInstances.forEach(component => component._onScrollWindow(event))
}

export function registerScrollListener (component) {
export function registerScrollListener (component, element = window) {
if (!mountedInstances.length) {
window.addEventListener('scroll', onScrollWindow)
element.addEventListener('scroll', onScrollWindow)
}
mountedInstances.push(component)
}

export function unregisterScrollListener (component) {
export function unregisterScrollListener (component, element = window) {
mountedInstances = mountedInstances.filter(c => (c !== component))
if (!mountedInstances.length) {
window.removeEventListener('scroll', onScrollWindow)
element.removeEventListener('scroll', onScrollWindow)
if (disablePointerEventsTimeoutId) {
clearTimeout(disablePointerEventsTimeoutId)
enablePointerEventsIfDisabled()
Expand Down
5 changes: 5 additions & 0 deletions source/demo/Application.css
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,11 @@ a {
flex-wrap: wrap;
justify-content: center;
}
.ScrollingBody {
composes: Body;
overflow: auto;
flex: 1 1 auto;
}
.column {
display: flex;
flex-direction: column;
Expand Down
28 changes: 25 additions & 3 deletions source/demo/Application.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,16 +42,38 @@ const list = Immutable.List(generateRandomList())

export default class Application extends Component {
static childContextTypes = {
list: PropTypes.instanceOf(Immutable.List).isRequired
list: PropTypes.instanceOf(Immutable.List).isRequired,
customElement: PropTypes.any,
isScrollingCustomElement: PropTypes.bool.isRequired,
setScrollingCustomElement: PropTypes.func
};

state = {
isScrollingCustomElement: false
}

constructor (props) {
super(props)
this.setScrollingCustomElement = this.setScrollingCustomElement.bind(this)
}

setScrollingCustomElement (custom) {
this.setState({ isScrollingCustomElement: custom })
}

getChildContext () {
const { customElement, isScrollingCustomElement } = this.state
return {
list
list,
customElement,
isScrollingCustomElement,
setScrollingCustomElement: this.setScrollingCustomElement
}
}

render () {
const { isScrollingCustomElement } = this.state
const bodyStyle = isScrollingCustomElement ? styles.ScrollingBody : styles.Body
return (
<HashRouter>
<div className={styles.demo}>
Expand Down Expand Up @@ -98,7 +120,7 @@ export default class Application extends Component {
</div>
</div>

<div className={styles.Body}>
<div className={bodyStyle} ref={e => this.setState({ customElement: e })}>
<div className={styles.column}>
<Match pattern='/wizard' component={Wizard} />
{Object.keys(COMPONENT_EXAMPLES_MAP).map((route) => (
Expand Down
2 changes: 1 addition & 1 deletion source/demo/ContentBox.css
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
.ContentBox {
flex: 1;
flex: 1 0 auto;
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 .Body). I spot checked the other demo changes and didn't see anything obviously broken. Removing overflow: auto has the same effect, if that is preferable to you.

Copy link
Owner

Choose a reason for hiding this comment

The 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;
Expand Down