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

Performances degrade with 200+ draggables #116

Closed
ramboInTheSky opened this issue Sep 25, 2017 · 12 comments
Closed

Performances degrade with 200+ draggables #116

ramboInTheSky opened this issue Sep 25, 2017 · 12 comments

Comments

@ramboInTheSky
Copy link

ramboInTheSky commented Sep 25, 2017

ScreenCapture_1.mp4.zip

Bug or feature request?

Bug?

Expected behaviour

I have noticed that with over 200 items in a Droppable, performances degrades a lot, especially when dragging between Droppables.

Actual behaviour

A draggable has a delay of half a second when first clicked&dragged around

Browser version

Chrome latest version

###DEMO

see attached

@tim-soft
Copy link

tim-soft commented Sep 25, 2017

Are you iterating over your entire lists of draggables on each drag, possibly many times?

@alexreardon alexreardon added this to the Fire Kingdom milestone Sep 26, 2017
@alexreardon
Copy link
Collaborator

Worth investigating why this occurs. I suspect it is because the draggable re-renders which causes all the children to render. So a simple thing to do would be to add a check in your Droppable should component update to see if snapshot.isDraggingOveris changing. If it is - try to avoid rendering out all your Draggables again.

You could create a child of your Droppable to manage this conditional rendering of your Draggables.

Another option would be to make your components that wrap your Draggable extend PureComponent so they only render if their props change.

This is my suspicion as to what is causing the issue - but we would need to investigate more.

@ramboInTheSky
Copy link
Author

ramboInTheSky commented Sep 26, 2017

Hi Alex,

I've done as you've suggested but with no joy.

I originally had everything on the same big component and I've actually noticed that the .map on my sources array was getting called several times (although the render was getting called only once) so I've extrapolated the droppable logic into a separate pure component and applied the shouldComponentUpdate logic you've suggested.

over 226 items I have a very few milliseconds improvement but the user perceived performances are basically the same as the video I've posted in the original thread.

here my code:

/Main Component:/
`import * as React from 'react'
import { DragDropContext, Droppable, Draggable } from 'react-beautiful-dnd';
import {DnDDraggable} from './draggable'
import {
Row,
Col,
ErrorBox,
bind,
Icon
} from 'common_lib'
import './index.css'

/* Interfaces and Types */
export interface DnDProps {
sources: T[]
destinations: T[]
itemsId: string
itemsName: string
itemsDescription?: string
className: string
sourcesName: string
destinationName: string
returnFn: Function
}

export interface DnDState{
sources: T[]
unfilteredSources: T[]
destinations: T[]
unfilteredDestinations: T[]
}

export class DnDTwoLists extends React.Component<DnDProps, DnDState> {
constructor(props: DnDProps) {
super(props)
bind(this)
this.state = {
sources: props.sources.sort(),
destinations: props.destinations.sort(),
unfilteredDestinations: props.destinations.sort(),
unfilteredSources: props.sources.sort()
}
}

onDragStart(initial: any){
document.body.classList.add('is-dragging')
document.getElementsByClassName('draggable_modal-wrapper')[0].classList.add('transform_none_fix')
}
onDragEnd(result: any){
document.body.classList.remove('is-dragging')
document.getElementsByClassName('draggable_modal-wrapper')[0].classList.remove('transform_none_fix')
if (!result.destination) {
return;
}

    let sources: T[] = Array.from(this.state.sources)
    let unfilteredSources: T[] = Array.from(this.state.unfilteredSources)
    let destinations: T[] = Array.from(this.state.destinations)
    let unfilteredDestinations: T[] = Array.from(this.state.unfilteredDestinations)

    if(result.destination.droppableId == result.source.droppableId ){
        if(result.destination.droppableId.indexOf('source') != -1){
            sources = this.reorder(
                this.state.sources,
                result.source.index,
                result.destination.index
            )
            unfilteredSources = this.reorder(
                this.state.unfilteredSources,
                result.source.index,
                result.destination.index
            )
        }
        else{
             destinations = this.reorder(
                this.state.destinations,
                result.source.index,
                result.destination.index
            )
            unfilteredDestinations = this.reorder(
                this.state.unfilteredDestinations,
                result.source.index,
                result.destination.index
            )
        }
    }
    else{
       if(result.destination.droppableId.indexOf('destination') != -1){
            const [removed] = sources.splice(result.source.index, 1)
            destinations.splice(result.destination.index, 0, removed)

            let unfilteredElementIndex
            const unfilteredElement:T | undefined = unfilteredSources.find((item: any, index)=>{
                unfilteredElementIndex = index
                return item[this.props.itemsId] == result.draggableId
            })
            if(unfilteredElement && unfilteredElementIndex){
                const [removed] = unfilteredSources.splice(unfilteredElementIndex, 1)
                unfilteredDestinations.splice(result.destination.index, 0, removed)
            }
       }
       else{
            const [removed] = destinations.splice(result.source.index, 1)
            sources.splice(result.destination.index, 0, removed)

            let unfilteredElementIndex
            const unfilteredElement:T | undefined = unfilteredDestinations.find((item: any, index)=>{
                unfilteredElementIndex = index
                return item[this.props.itemsId] == result.draggableId
            })
            if(unfilteredElement && unfilteredElementIndex){
                const [removed] = unfilteredDestinations.splice(unfilteredElementIndex, 1)
                unfilteredSources.splice(result.destination.index, 0, removed)
            }
       }
    }

    this.props.returnFn(unfilteredDestinations)

    this.setState({
        sources,
        destinations,
        unfilteredSources,
        unfilteredDestinations
    })
}

// a little function to help us with reordering the result
reorder(list: T[], startIndex: number, endIndex: number ):T[]{
    const result = Array.from(list)
    const [removed] = result.splice(startIndex, 1)
    result.splice(endIndex, 0, removed)

    return result as T[]
}

getListStyle(isDraggingOver: any){
    return{
        background: isDraggingOver ? 'lightblue' : '#fff',
        padding: 10,
    }
}

getItemStyle(draggableStyle: any, isDragging: any){
    return {
        // some basic styles to make the destinations look a bit nicer
        userSelect: 'none',
        cursor: 'pointer',
        // change background colour if dragging
        background: isDragging ? '#00746B' : '#fff',
        color: isDragging ? '#fff' : '#333',
        // styles we need to apply on draggables
        ...draggableStyle
    }
}

filterSources(val: any){
    if(val && val.currentTarget.value != ''){
        const sources = Array.from(this.state.unfilteredSources)
        const filtered = sources.filter((item: any)=>{
                return item[this.props.itemsName].toLowerCase().includes(val.currentTarget.value.toLowerCase())
        })
        this.setState({
            sources: filtered
        })
    }
    else{
        this.setState({
            sources: this.state.unfilteredSources
        })
    }
}

filterDestinations(val: any){
    if(val && val.currentTarget.value != ''){
        const destinations = Array.from(this.state.unfilteredDestinations)
        const filtered = destinations.filter((item: any)=>{
                return item[this.props.itemsName].toLowerCase().includes(val.currentTarget.value.toLowerCase())
        })
        this.setState({
            destinations: filtered
        })
    }
    else{
        this.setState({
            destinations: this.state.unfilteredDestinations
        })
    }
}

clearFilterSources(e: any){
    console.log(e)
    e.preventDefault()
    e.currentTarget.previousElementSibling.value = ''
    this.filterSources(null)
}

clearFilterDestinations(e: any){
    console.log(e)
    e.preventDefault()
    e.currentTarget.previousElementSibling.value = ''
    this.filterDestinations(null)
}

moveAllToDest(){
    let filtersArray = document.getElementsByClassName('draggableList-filter') as HTMLCollectionOf<HTMLInputElement>
    filtersArray[0].value = filtersArray[1].value = ''
    this.setState({
        destinations: this.state.unfilteredSources,
        unfilteredDestinations : this.state.unfilteredSources,
        unfilteredSources: [],
        sources: []
    })
}

moveAllToSource(){
    let filtersArray = document.getElementsByClassName('draggableList-filter') as HTMLCollectionOf<HTMLInputElement>
    filtersArray[0].value = filtersArray[1].value = ''
    this.setState({
        unfilteredSources: this.state.unfilteredDestinations,
        sources: this.state.unfilteredDestinations,
        destinations: [],
        unfilteredDestinations: []
    })
}

render(){
    const {itemsId, itemsName, itemsDescription, className, sourcesName, destinationName} = this.props
    const {sources, destinations} = this.state
    const onDragStart = this.onDragStart
    const onDragEnd = this.onDragEnd
    const getListStyle = this.getListStyle
    const getItemStyle = this.getItemStyle
    const filterSources = this.filterSources
    const clearFilterSources = this.clearFilterSources
    const filterDestinations = this.filterDestinations
    const clearFilterDestinations = this.clearFilterDestinations
    const moveAllToDest = this.moveAllToDest
    const moveAllToSource = this.moveAllToSource

    return(
        <DragDropContext onDragStart={this.onDragStart} onDragEnd={this.onDragEnd}>
            <div>
            <div className="dnd_list-container">
                <h2>{sourcesName}</h2>
                <div>
                    <input type="text" className="draggableList-filter" onChange={filterSources} placeholder="Filter..."/>
                    <a className="draggableList-cancelFilter" onClick={clearFilterSources}><Icon name="times"/></a>
                </div>
                <div className="dnd_list">
                    
                    <Droppable droppableId={`${className}_source`} >
                    {(provided: any, snapshot: any) => (
                        <div
                            ref={provided.innerRef}
                            style={getListStyle(snapshot.isDraggingOver)}
                        >
                           <DnDDraggable snapshot={snapshot} sources={sources} itemsId={itemsId} itemsName={itemsName} itemsDescription={itemsDescription} getItemStyle={getItemStyle}/>
                        {sources.length ==0? <h2>Drop your items here</h2> : null}
                        {provided.placeholder}
                        </div>
                    )}
                    </Droppable>
                </div>
            </div>

            <div className="dnd_list-container-separator">
                <div onClick={(e)=>moveAllToSource()} style={{marginBottom:20}}> &lt;&lt; </div>
                <div onClick={(e)=>moveAllToDest()}> &gt;&gt; </div>
            </div>


            <div className="dnd_list-container">
                <h2>{destinationName}</h2>
                <div>
                    <input type="text" className="draggableList-filter" onChange={filterDestinations} placeholder="Filter..."/>
                    <a className="draggableList-cancelFilter" onClick={clearFilterDestinations}><Icon name="times"/></a>
                </div>
                <div className="dnd_list">
                    
                    <Droppable droppableId={`${className}_destination`} >
                    {(provided: any, snapshot: any) => (
                        <div
                            ref={provided.innerRef}
                            style={getListStyle(snapshot.isDraggingOver)}
                        >
                            <DnDDraggable snapshot={snapshot} sources={destinations} itemsId={itemsId} itemsName={itemsName} itemsDescription={itemsDescription} getItemStyle={getItemStyle}/>
                        {destinations.length ==0 ? <h2>Drop your items here</h2> : null}
                        {provided.placeholder}
                        </div>
                    )}
                    </Droppable>
                </div>
            </div>
        </div>
    </DragDropContext>

    )
}

}
`

/Draggable Component:/

`import * as React from 'react'
import { Draggable } from 'react-beautiful-dnd';

export interface DnDDraggableProps{
sources: any
itemsId: string
getItemStyle: Function
itemsDescription?: any
itemsName: string
snapshot: any
}

export class DnDDraggable extends React.PureComponent<DnDDraggableProps, {}> {

shouldComponentUpdate(nextProps: DnDDraggableProps){
return nextProps.snapshot.isDraggingOver == this.props.snapshot.isDraggingOver
}

render(){
const {sources, getItemStyle, itemsDescription, itemsId, itemsName} = this.props

return(
    <div>
    {sources.map((item: any)=> (
        <Draggable key={item[itemsId]} draggableId={item[itemsId]}>
            {(provided: any, snapshot: any) => {
                
                return (
                <div className="draggable_item">
                    <div
                        className="draggable_item-inner"
                        ref={provided.innerRef}
                        style={getItemStyle(
                            provided.draggableStyle,
                            snapshot.isDragging
                        )}
                        {...provided.dragHandleProps}
                    >
                        <p className="dnd-cardTitle">{item[itemsName]}</p>
                        {itemsDescription && item[itemsDescription]?
                        <p className="dnd-cardDescription">
                            {typeof item[itemsDescription] == 'object' ? item[itemsDescription].join(', ') : item[itemsDescription]}
                        </p>:null}
                    </div>
                    {provided.placeholder}
                </div>
            )} }
        </Draggable>
    ))}
    </div>
)

}

}`

@OmriAharon
Copy link

I tried, unfortunately to no avail.

@alexreardon
Copy link
Collaborator

@OmriAharon please post a webpack bin with your scenario. Here is a basic example to start with: https://www.webpackbin.com/bins/-Kr9aE9jnUeWlphY8wsw

Looking at all the code pasted it is hard to know what is going on

@alexreardon
Copy link
Collaborator

Just to clarify:

When you drag over a Droppable it will re-render. This will cause the all the children to re-render (expensive) unless you block the render. You can achieve this through a variety of mechanisms. The simpliest would be for your component that wraps a Draggable to extend PureComponent. This will stop it rendering when its props did not change.

Your Droppable may wrap a container component. This one could contain the custom shouldComponentUpdate or extend PureComponent to block the render of all the children. Or you can do it on the components wrapping the Draggable themselves. You could also apply this blocking to the child of your Draggable. Keep in mind that the higher up you block the render the faster it will be

@alexreardon
Copy link
Collaborator

Unfortunately the library itself cannot apply this logic - it is a restriction of the function as child pattern: https://medium.com/merrickchristensen/function-as-child-components-5f3920a9ace9

@OmriAharon
Copy link

OmriAharon commented Sep 27, 2017

Thanks @alexreardon. The code above is not my own, I'm aware it's not feasible to track something like this. In your suggested WebpackBin, I tried simply with 1000 items which is stuck (I know it's a lot, that's the case in my app). I'll keep trying to do as you suggest in my app and come up with a solution.

@ramboInTheSky
Copy link
Author

Hi Alex, All,

I've tried with this with no joy, am I doing it wrong?
https://www.webpackbin.com/bins/-Kv2JHj3XRGM1Btk5oJl

even on the basic scenarion
https://www.webpackbin.com/bins/-Kr9aE9jnUeWlphY8wsw
replacing with 300 elements performances degrade as is.

@ramboInTheSky
Copy link
Author

Well first of all thanks for replying and keeping up with my issue guys, I really think the lib does a great job.
I've also tried blocking completely the re-render of both draggable and droppable but the performance issue persists.
https://www.webpackbin.com/bins/-Kv2Jxkg0dBC6er_vcSM

@OmriAharon
Copy link

@varHarrie Would really like to see a webpackbin of your successful scenario.

@alexreardon
Copy link
Collaborator

I misunderstood the initial issue. I thought it was related to moving into a new list.

It is actually:

"A draggable has a delay of half a second when first clicked&dragged around"

This is already being tracked here: #86

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants