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

Mutating this.props.layouts #178

Closed
kibs opened this issue Mar 2, 2016 · 11 comments
Closed

Mutating this.props.layouts #178

kibs opened this issue Mar 2, 2016 · 11 comments

Comments

@kibs
Copy link

kibs commented Mar 2, 2016

I am maybe doing something wrong, but I am trying to make an editor where i provide the layouts using the "layouts" props from my internal state. I then listen for changes using onLayoutChange, and update my own state.layouts which is then propagated down to the ResponsiveGrid component. Anyway

https://github.com/STRML/react-grid-layout/blob/master/lib/ResponsiveReactGridLayout.jsx#L130-L142

It looks like this code is mutating the props.layouts object, and therefore my state object is mutated in place. Is this by design. It makes it hard to create an "controlled" component.

Does my question even make sense :)

@olslash
Copy link

olslash commented Mar 17, 2016

+1 -- this issue (the fact that the library depends on holding/managing/mutating the current layout in its own state) makes it nearly impossible make a decent implementation using redux or similar. If anyone's done this, I would love to hear how.

belive this was also discussed in #156

@olslash
Copy link

olslash commented Mar 17, 2016

I managed to get something decent working by

  • not letting my reducer update the modified layout if it hadn't actually changed. The library was firing off tons of useless onLayoutChange calls with identical layouts which would cause infinite update loops.
  • deep-cloning the layout passed in by onLayoutChange before putting it in state, so the library can't mutate it.

still a bit hacky, and i'd love to see the library take a better approach to state in the future

@STRML
Copy link
Collaborator

STRML commented Mar 17, 2016

I'm with you on this - would like to do a similar approach to what I did with <Draggable>, where we expose a Core component that has no state, or make a breaking change and create a HoC like WidthProvider that does the state management.

Then consumers will simply have to listen to onLayoutChange and assign it back via state, or modify it... or do whatever they like.

@olslash
Copy link

olslash commented Mar 17, 2016

great, yeah, i think for nontrivial cases most people are going to want to manage the state themselves -- having a HOC that provides the same "just works" behavior, + a stateless core would be a great approach.

@kibs
Copy link
Author

kibs commented Mar 18, 2016

Cool, i think it will clean up a lot of code when you don't have to keep state yourself. Making it more stable. And also, "everyone" in the React community is used to and expects components to work this way.

@jonashartwig
Copy link

Hi, is there anyone working on this?

@STRML
Copy link
Collaborator

STRML commented Oct 3, 2016

The library shouldn't be mutating the input layouts item anymore. It clones it and then goes to town. Please comment if you are still seeing this bad behavior.

@arunkjn
Copy link

arunkjn commented Oct 4, 2016

@STRML this is great. Is the change present in release 0.13.7 ?

@STRML
Copy link
Collaborator

STRML commented Oct 4, 2016

Yeah.

On Oct 4, 2016 2:57 AM, "Arun Kumar Jain" notifications@github.com wrote:

@STRML https://github.com/STRML this is great. Is the change present in
release 0.13.7 ?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#178 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABJFPy5jgRiayQxWp-hVUyj-6iJsUiqPks5qwgbZgaJpZM4HnxUw
.

@jonashartwig
Copy link

so i have been testing this a lot now and it seems to be good :)

@kibs kibs closed this as completed Nov 23, 2016
@vasilevach
Copy link

vasilevach commented Feb 5, 2020

Hi guys, sorry to "reopen" this issue, but 0.17.1 still mutates the state. It adds some new breakpoints when non are defined. I would also love a version, where the component is stateless and we can manage our own state. Outside drag is ideal candidate for such a behavior.

Other than that, I have another faulty behavior: This is my component definition:

<ResponsiveReactGridLayout
        breakpoints={{ lg: 996, md: 768, sm: 480, xs: 0, xxs: 0 }}
        className={classes.dropArea}
        cols={{ lg: 12, md: 10, sm: 6, xs: 4, xxs: 2 }}
        compactType="vertical"
        draggableHandle={`.${DRAGGABLE_COMPONENT_CLASS}`}
        droppingItem={droppingItem}
        isDroppable
        layouts={clonedLayouts}
        measureBeforeMount={false}
        rowHeight={150}
        useCSSTransforms={mount}
        onBreakpointChange={onBreakpointChange}
        onDrop={onNewElement}
        onLayoutChange={onLayoutChange}
        onWidthChange={(...args) => {}}
      >
        {renderLayout}
      </ResponsiveReactGridLayout>

What I experience as problem is that droppingItem is mutating my Redux state (without me wanting so via the onLayoutChange, and even worse: it is doing it twice consecutive times: once with the new item in place ("dropping-elem") and once without it. Needless to say, I cannot see my droppingItem in the layout because of it. I would benefit greatly if the onLayoutChange is more verbose on the changes it makes. I will use it for the resize, but I would skip it for the dropping item rearrangements. If someone has been stuck here, I would greatly appreciate the help :) I don't feel like forking the library and moding it locally. Not good for sustainability :)

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

No branches or pull requests

6 participants