-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
fix: dynamic isDraggable and isResizable remount #892
Conversation
@STRML any update? |
This PR will also fix #626 |
Made another way to do it #937 |
Another ping for this PR to get looked at. This is a pretty major blocker for many scenarios. |
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. |
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? |
@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. |
This can be done for resizability per item by hiding the handle via CSS, and for grid items by returning |
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 |
Nudge... |
Great to see progress... What is the "Labeler / label (pull_request)" check that's failing? |
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 |
@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? |
The above worked almost perfectly, but since the 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. 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 #89 <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;
}
`}
}
` |
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. |
Will be released in a few minutes in |
Fixes #240
Fixes #892
Issue:
Childrens are remounted when isDraggable or isResizable is changed.
Solution:
The
Resizable
andDraggableCore
parents are always mounted to prevent remount of children.Use
disabled
to prevent calling drag handlers and hide the resizable handle with css.