-
-
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 15 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 |
---|---|---|
@@ -0,0 +1,3 @@ | ||
.disabled > * { | ||
pointer-events: none !important; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
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 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 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 commentThe 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 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. One way I test things like this is by iterating over a set of options and creating my tests (the 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> | ||
|
@@ -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' | ||
value={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 }) => ( | ||
|
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() | ||
unregisterScrollListener(this, this.scrollElement) | ||
registerScrollListener(this, nextProps.scrollElement) | ||
} else if (!nextProps.scrollElement && this.scrollElement !== window) { | ||
this._updateDimensions() | ||
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 () { | ||
const { height } = this.state | ||
|
||
this._positionFromTop = getPositionFromTop(ReactDOM.findDOMNode(this), this.scrollElement) | ||
|
||
const newHeight = getHeight(this.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 @@ | ||
import styles from '../WindowScroller.css' | ||
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. Not using CSS modules in RV proper |
||
let mountedInstances = [] | ||
let originalBodyPointerEvents = null | ||
let disablePointerEventsTimeoutId = null | ||
|
||
/** | ||
|
@@ -8,57 +8,52 @@ let disablePointerEventsTimeoutId = null | |
*/ | ||
export const IS_SCROLLING_TIMEOUT = 150 | ||
|
||
function enablePointerEventsIfDisabled () { | ||
function enablePointerEventsIfDisabled (element) { | ||
if (disablePointerEventsTimeoutId) { | ||
disablePointerEventsTimeoutId = null | ||
|
||
document.body.style.pointerEvents = originalBodyPointerEvents | ||
|
||
originalBodyPointerEvents = null | ||
element.classList.remove(styles.disabled) | ||
} | ||
} | ||
|
||
function enablePointerEventsAfterDelayCallback () { | ||
enablePointerEventsIfDisabled() | ||
function enablePointerEventsAfterDelayCallback (element) { | ||
enablePointerEventsIfDisabled(element) | ||
mountedInstances.forEach(component => component._enablePointerEventsAfterDelayCallback()) | ||
} | ||
|
||
function enablePointerEventsAfterDelay () { | ||
function enablePointerEventsAfterDelay (element) { | ||
if (disablePointerEventsTimeoutId) { | ||
clearTimeout(disablePointerEventsTimeoutId) | ||
} | ||
|
||
disablePointerEventsTimeoutId = setTimeout( | ||
enablePointerEventsAfterDelayCallback, | ||
IS_SCROLLING_TIMEOUT | ||
) | ||
enablePointerEventsAfterDelayCallback, | ||
IS_SCROLLING_TIMEOUT, | ||
element | ||
) | ||
} | ||
|
||
function onScrollWindow (event) { | ||
if (originalBodyPointerEvents == null) { | ||
originalBodyPointerEvents = document.body.style.pointerEvents | ||
const disabledElement = event.currentTarget === window ? document.documentElement : event.currentTarget | ||
disabledElement.classList.add(styles.disabled) | ||
|
||
document.body.style.pointerEvents = 'none' | ||
|
||
enablePointerEventsAfterDelay() | ||
} | ||
enablePointerEventsAfterDelay(disabledElement) | ||
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() | ||
enablePointerEventsIfDisabled(element === window ? document.documentElement : element) | ||
} | ||
} | ||
} | ||
|
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’m using this instead of inline styles because a custom scroll container may have many children that need
pointer-events: none
, whereas scrolling the window only needs to apply that style tobody
. So, I wasn’t sure whether to add!important
or not. It ought to work pretty similarly to the way it used to (replacing inline styles), and it is still overrideable (higher-specificity selectors with!important
, or inline styles with!important
). On the other hand, I hate it when libraries use!important
.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.
Do you actually mean
.disabled *
?Hm... either way,
> *
is not a very performant selector so I'm pretty reluctant to use it, and definitely not with such a generic class name asdisabled
. RV classes are named so as to (hopefully) not conflict with any other global CSS names.Also related- RV has also been shifting away from any CSS-based styles (with the exception of
Table
). Basically anything that's functional is defined via an inline style; anything that's presentational (onlyTable
) is defined via CSS to make it easier to override in a variety of ways (without requiring use of!important
).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.
If I understand correctly, the intention is to disable pointer events on the contents of whatever element is scrolling, without disabling them on the scrolling element itself. So if the contents can now be N number of nodes, either we use CSS to select all children, or we iterate over all N nodes to add/remove inline styles.
To the initial question,
.disabled > *
and.disabled *
should have the same effect, and I didn't expect the latter to be more efficient, but that’s fine.And as far as the name goes, we can definitely change it—since the demo was using CSS Modules, and I'm using CSS Modules in my own stuff, I totally didn't think about it. Another option would be to dynamically generate/insert a stylesheet, but that would be a lot more magic; harder for developers to track down if they have the need to customize behavior.
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.
No. The first disables pointer events only for immediate children. The latter disables for all descendants. I wasn't sure which you were going for. (The second is more expensive, so I wasn't recommending it- just asking for clarification.) First would only work if the
Grid
(orList
or whatever) was an immediate child of the newscrollElement
.Yup, the demo uses CSS modules (love 'em) but the lib itself doesn't in order to be as flexible and compatible with the various approaches to styling as possible.
Agreed. I don't think this is a good path to go down.
Hmm...it's been a long day and I'm tired, but...I wonder if it would be more appropriate to approach this by passing a prop (eg
disablePointerEvents
) toGrid
telling it to disable pointer events. That would be more performant than this CSS approach. Basically this line would change to something like: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.
Well, yeah, but
pointer-events
is inherited, which I guess is what I was counting on there—analogous to WindowScroller in master counting on elements to inherit it fromdocument.body
.That works, but it is different than the existing behavior. WindowScroller in current
master
disables pointer-events ondocument.body
, e.g. all the stuff inside the scroll container. My CSS (while obviously not being feasible) accomplishes exactly that for arbitrary scroll containers. What you’re suggesting would narrow the scope of what getspointer-events: none
down to just the grid/list, not the whole scroll container. That seems like a reasonable trade-off to me, but I don’t have the same prior knowledge as you about howpointer-events
effects performance of this whole system. If you’re ok with that trade-off, I can move forward with implementing it.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.
pointer-evetns
is an Inherited style, yes, but it can be overridden by children. AndGrid
would override, since it sets the property on theReactVirtualized__Grid__innerScrollContainer
, which is not the direct child ofWindowScroller
but- at best- a child of its child.Also using
!important
is kind of a last resort, so I like to avoid it.