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

Rendering with new Breakpoints props do not refresh the layout #208

Closed
damienleroux opened this issue Apr 10, 2016 · 6 comments
Closed

Rendering with new Breakpoints props do not refresh the layout #208

damienleroux opened this issue Apr 10, 2016 · 6 comments

Comments

@damienleroux
Copy link
Contributor

Hello,

I was using react-grid-layout on a responsive page and I made this page configurable in live by user. it means a user can see how the layout is displayed depending on how he has configured breakpoints. It is some kind of layout builder using react grid layout if you know what I mean. If the code is worth it at the end, I'll be happy to share it on github. The problem is that when React-grid-layout is rendered, if i fill it with a new props "breakpoints", nothing refreshes. I am under the impression that the function used to refresh the layout is called only when initializing the state: https://github.com/STRML/react-grid-layout/blob/master/lib/ResponsiveReactGridLayout.jsx#L81-L83 and should be called in componentWillReceiveProps when receiving a new breakpoints props too.

What do you think?

Thank you for your advice

@damienleroux
Copy link
Contributor Author

I just want to add that the problem is identical for the props "cols"

@STRML
Copy link
Collaborator

STRML commented Apr 15, 2016

You're correct, there should be hooks to refresh the state based on this. A memoized selector like reselect may be a better way to construct these, rather than putting them in state at all.

@damienleroux
Copy link
Contributor Author

Yeah I agree, It is always tricky to keep state up to date with props info. I just find out another issue in ResponsiveReactGridLayout when injecting new width through props:

In ResponsiveReactGridLayout.componentWillReceiveProps():
onWithChange is called to recalculate responsive data. In the process, a variable layouts references the layouts from next props. Then this layout is modified with a new layout. At the end of onWithChange, setState() is done : that is important to set the new breakpoint calcultated ( going from lg to sm for example). But when onWithChange ends and we come back in componentWillReceiveProps(), the statement isEqual(nextProps.layouts, this.props.layouts) can return true and therefore findOrGenerateResponsiveLayout is triggered. findOrGenerateResponsiveLayout() does calculs reading breakpoints on this.state. That is the problem! this.state.breakpoints is yet lg and not sm because componentWillReceiveProps() must ends to apply setState().

Actually, the problem is that this line should not read beakpoint and cols on the state because the state is not yet updated at this point.

I think that onWidthChange should return the new data beakpoint and cols as for layouts.

Could you provide fix on those points? or do you prefer i try a pull request?

Thank you a lot for your work! I think it is totally amazing! I am trying to implement a live UI configurator that will allow a user to modify the responsive behavior of react-grid-layout in live!

@STRML
Copy link
Collaborator

STRML commented Apr 15, 2016

Thanks. I suspect you'll run into far more of these until I properly remove
most state. I'll take a look but if you get impatient I always review PRs.
On Apr 15, 2016 4:09 PM, "damienleroux" notifications@github.com wrote:

Yeah I agree, It is always tricky to keep state up to date with props
info. I just find out another issue in ResponsiveReactGridLayout when
injecting new width through props:

In ResponsiveReactGridLayout.componentWillReceiveProps():
onWithChange
https://github.com/STRML/react-grid-layout/blob/master/lib/ResponsiveReactGridLayout.jsx#L95
is called to recalculate responsive data. In the process, a variable
layouts
https://github.com/STRML/react-grid-layout/blob/master/lib/ResponsiveReactGridLayout.jsx#L132
references the layouts from next props. Then this layout is modified with
a new layout
https://github.com/STRML/react-grid-layout/blob/master/lib/ResponsiveReactGridLayout.jsx#L144.
At the end of onWithChange
https://github.com/STRML/react-grid-layout/blob/master/lib/ResponsiveReactGridLayout.jsx#L95,
setState() is done : that is important to set the new breakpoint
calcultated ( going from lg to sm for example). But when onWithChange
ends and we come back in componentWillReceiveProps(), the statement isEqual(nextProps
.layouts, this.props.layouts)
https://github.com/STRML/react-grid-layout/blob/master/lib/ResponsiveReactGridLayout.jsx#L99
can return true and therefore findOrGenerateResponsiveLayout
https://github.com/STRML/react-grid-layout/blob/master/lib/ResponsiveReactGridLayout.jsx#L104
is triggered. findOrGenerateResponsiveLayout() does calculs reading
breakpoints on this.state. That is the problem! this.state.breakpoints is
yet lg and not sm because componentWillReceiveProps() must ends to apply
setState().

Actually, the problem is that this line
https://github.com/STRML/react-grid-layout/blob/master/lib/ResponsiveReactGridLayout.jsx#L100
should not read beakpoint and cols on the state because the state is not
yet updated at this point.

I think that onWidthChange should return the new data beakpoint and cols
as for layouts.

Could you provide fix on those points? or do you prefer i try a pull
request?

Thank you a lot for your work! I think it is totally amazing! I am trying
to implement a live UI configurator that will allow a user to modify the
responsive behavior of react-grid-layout in live!


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#208 (comment)

@STRML
Copy link
Collaborator

STRML commented May 3, 2016

0.12.3 should address this. I'm going to make a new issue for the state issue in general.

@damienleroux
Copy link
Contributor Author

damienleroux commented May 3, 2016

Thank you. It works fine on my side!

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

2 participants