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

fix: dynamic isDraggable and isResizable remount #892

Merged
merged 2 commits into from Feb 10, 2020
Merged

fix: dynamic isDraggable and isResizable remount #892

merged 2 commits into from Feb 10, 2020

Conversation

lmachens
Copy link

@lmachens lmachens commented Dec 19, 2018

Fixes #240
Fixes #892

Issue:
Childrens are remounted when isDraggable or isResizable is changed.

Solution:
The Resizable and DraggableCore parents are always mounted to prevent remount of children.
Use disabled to prevent calling drag handlers and hide the resizable handle with css.

@lmachens
Copy link
Author

@STRML any update?

@kravets-levko
Copy link

This PR will also fix #626

@artemsaveliev
Copy link

Made another way to do it #937
Not speaking for maintainer, but I am not sure switching to always wrapping with DragggableCore is good. In my version you can still remove DraggableCore by setting isDraggable off.

@nevalera
Copy link

nevalera commented Jun 6, 2019

Any updates on this or #937 @STRML ?

The fix for the remounting issue is required for us to be able to use this library.

@agg23
Copy link

agg23 commented Sep 16, 2019

Another ping for this PR to get looked at. This is a pretty major blocker for many scenarios.

@STRML
Copy link
Collaborator

STRML commented Sep 16, 2019

What performance impact will this have? I am also loathe to always wrap elements when not needed. I would prefer the user to manually hide the handles if they have the desire to make draggability/resizability dynamic.

@asaveliev
Copy link

I would prefer the user to manually hide the handles if they have the desire to make draggability/resizability dynamic

Currently entering/exiting editing triggers complete reload of all widgets - depending on scenario, this may not be a great experience. Specifically for us it is expensive as widgets have expensive rendering paths.

I am not sure there's a way to make draggability dynamic with current version - other than switching each widget to static?

@n1ghtmare
Copy link
Collaborator

What performance impact will this have? I am also loathe to always wrap elements when not needed. I would prefer the user to manually hide the handles if they have the desire to make draggability/resizability dynamic.

@STRML As far as performance impact, what are your concerns here? Otherwise, I actually tend to agree with you on the second part (in the sense that, we shouldn't clutter the code-base with various features that can be achieved with some code on the user's side). However in this case I think there is a good use case as others stated.

@STRML
Copy link
Collaborator

STRML commented Sep 17, 2019

This can be done for resizability per item by hiding the handle via CSS, and for grid items by returning false from the grid's onDragStart(layout, oldItem, newItem, placeholder, e, ?node) callback.

@agg23
Copy link

agg23 commented Sep 17, 2019

While that is good to know, I don't really consider those solutions to be much more than hacks. In general, I expect React components to dynamically respond to configuration updates with only minor rerendering; that is one of the core constructs of React. The fact that there are isDraggable and isResizable props that require such hacks if you want them to respond in the "expected" way indicates to me that something more should be done in this area.

@mitulmk
Copy link

mitulmk commented Oct 9, 2019

Nudge...

@STRML STRML added bug core use this label for changes in `lib` directory labels Oct 9, 2019
@mitulmk
Copy link

mitulmk commented Oct 14, 2019

Great to see progress... What is the "Labeler / label (pull_request)" check that's failing?

@github-actions
Copy link

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this PR will be closed in 7 days

@github-actions github-actions bot added the stale The label to apply when a pull request or an issue is stale label Nov 13, 2019
@agg23
Copy link

agg23 commented Nov 13, 2019

@STRML, are you strongly opposed to this change? As it stands, is there any likelihood that this gets approved, or should this PR be closed?

@JohnDDuncanIII
Copy link

JohnDDuncanIII commented Feb 10, 2020

This can be done for resizability per item by hiding the handle via CSS, and for grid items by returning false from the grid's onDragStart(layout, oldItem, newItem, placeholder, e, ?node) callback.

The above worked almost perfectly, but since the GridItem.jsx onDragStart computes and sets the dragging state value before it invokes this.props.onDragStart && this.props.onDragStart(, this meant that if I clicked on any disabled dashboard widget when not in edit mode, then react-grid-layout would still append the react-draggable-dragging className to the widget.

This caused a particularly nasty issue where any widget that had been clicked before we entered edit mode would get "stuck" in its "dragging" position until you dragged it away and then back in. If you tried to drag a non-stuck widget over the stuck one, they would just overlap with each other.

Screen Shot 2020-02-10 at 3 46 20 PM

To fix this, I took the aforementioned advice from STRML and also utilized some techniques outlined in all of the related issues to reach what I would consider to be the current "canonical" solution to the unmount/remount bug that occurs when you change isDraggable/isResizable

#89
#240
#362
#371
#379
#626
#721
#756
#1067

<StyledReactGridLayout
    ...
    // this completely disables the drag functionality for a GridItem
    draggableHandle={!this.props.editing ? ".not-draggable" : null}
    // this is our internal redux store value that tracks whether we have clicked the edit button
    editing={this.props.editing}
    // these should always be set to true to prevent remounts
    isDraggable
    isResizable
    // if we are editing, we want the default onDragStart functionality
    // if we are not editing, then we return false to disable the internal GridItem onDrag
    onDragStart={() => this.props.editing}
    ...
>
    {...}
</StyledReactGridLayout>
import styled, { css } from "styled-components"
import RGL, { WidthProvider as provideWidth } from "react-grid-layout"
const ReactGridLayout = provideWidth(RGL)

export const StyledReactGridLayout = styled(ReactGridLayout)`
    position: relative;
    transition: height 100ms ease;

    .react-grid-item {
        box-sizing: border-box;
        transition: all 100ms ease;
        transition-property: left, top;

        .react-resizable-handle {
            display: none;
        }

        ${props => (props.editing) && css`
            .react-resizable-handle {
                display: initial;
            }

            &:hover {
                cursor: pointer;
            }

            &:active {
                background: #dadada;
                cursor: move;
            }
        `}
    }
`

@STRML
Copy link
Collaborator

STRML commented Feb 10, 2020

Alright. I think yall have convinced me that this is worth the rather small bit of overhead that is added in cases where you are not using draggability/resizability. I would rather that this component be easy to use. Merging. I'll cut a release in the next few days when I have time to prepare the changelog and do extra regression testing.

@STRML STRML merged commit 59b0da6 into react-grid-layout:master Feb 10, 2020
@lmachens lmachens deleted the lm-fix-dynamic-remount branch February 11, 2020 16:20
@STRML
Copy link
Collaborator

STRML commented Feb 25, 2020

Will be released in a few minutes in 0.18.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug core use this label for changes in `lib` directory stale The label to apply when a pull request or an issue is stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dynamic isDraggable isResizable
10 participants