danhaggard / react-scroll-collapse Goto Github PK

In collapserWrapper.js - rather than using the allChildItems selector - which returns an array of ids. Create a selector that returns the array of state objects for the items. So that you don't have to add in information into the action in the the itemsReducer.

And in the interest of defining different callbacks in the future - give the action a more accurate name:

COLLAPSE_ALL_IF_ALL_EXPANDED_ELSE_EXPAND_ALL

If a newly mounted collapserController has a parent collapserController - then the reducers need to do two things - update the state of the existing collapserController to register it as the parent; and add the new child collapserController into state.

Having one action for this is leading to code splitting where different reducers are handling the same state slice. See code here.

Instead of having logic checking if there a parentCollapser supplied in the action - do that back in the view layer and only fire the appropriate action if needed.

New action could be: ADD_COLLAPSER_PARENT

edit: same issue with REMOVE_COLLAPSER.

See the code for autoScroller.

The resulting functional HoC - would wrap Scroller thusly: scrollerMotion(Scroller) - which in turn could be wrapped by connect.

The componentDidUpdate method of the Scroller component - and componentWillReceiveProps of ScrollerMotion have both been used to solve scrolling animation edge case bugs in an adhoc way - by a complex series of prev/next props conditionals.

Each of the edge cases involves the underlying DOM element properties getting out of sync with application state.

Code needs to be consolidated into one component - and the underlying causes tracked down so that they can be dealt with more systematically - preferably outside of the react life-cycle methods. Putting this all into one issue because each edge case could be impacting the others.

Question: causes of each edge case?
Question: alternative solutions?

Edge case 1 - User initiated scroll event de-syncs redux and ui scrollTop state

The react-motion component always uses at its starting point the scrollTop value passed to it in the previous render. A user scroll event doesn't cause a render. So when the next animation occurs - react-motion will use a distant value causing a large jump in the scrolling.

Detected in in willReceiveProps

Current fix: calls setState to set motionStyle without a spring. See commit This forces a render in ReactMotion without interpolation that sets it's next starting point to the current scrollTop. Infinite renders prevented with state.prevRenderType flag.

Edge case 2 - new scrollTop prop not passed on init

When scrollTop === 0 (usually on init) - an ExpandCollapse/All dispatch that causes an auto-scroll should result in the new scrollTop value being passed back to ScrollerMotion by the sagas (through redux) - but it doesn't, causing UI de-sync

Detected in willReceiveProps

Same fix as case 1. Force ui-sync re-render without spring.

Edge case 3 - change in offsetTop prop does not precipitate re-render

(To replicate - disable the condition linked below. Then if you open and close the same nested collapser repeatedly and then click one of the nested collapsers without scrolling - the failed render will be noticeable).

Only noticed this after introducing fix for case 2. So could be caused by the current fix for 2.

Handled in <ScrollerMotion willReceiveProps:

Edge case 4 - Things like viewport resize can change the ui scrollTop value - causing de-sync

This is currently being handled in Scroller.componentDidUpdate. and surely must be compounding the other edge cases. Should responsibility for setting the dom scrollTop value be moved to ScrollerMotion? - Scroller could then become entirely dumb.