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

Pivot component does not store configuration when re-rendered #101

Open
GerjonM opened this issue Oct 30, 2017 · 5 comments
Open

Pivot component does not store configuration when re-rendered #101

GerjonM opened this issue Oct 30, 2017 · 5 comments

Comments

@GerjonM
Copy link

GerjonM commented Oct 30, 2017

My table includes an onCellClick method which, when triggered, opens a modal in the UI which shows the cell details. The problem here is that when I close the modal, the table reverts back to its initial state and I have to build it again.

Currently I am solving this by wrapping the Pivot element in a wrapper with the shouldComponentUpdate class returning false, making sure the Pivot element does not re-render. However I think it is preferable to make the component only reset when it receives different props, or to introduce an onChange prop that gets a representation of the state and a way to pass this state back in again as a prop. This would also allow developers to store previous configurations.

@turnerniles
Copy link
Owner

@GerjonM It seems right now whenever the Pivot componentWillReceiveProps is triggered it will re-render with the props it is passed and this essentially wipes out / resets the current pivot (aside from a change in the colorPack props which it handles and doesn't wipe out / reset the current pivot).

Are you doing a onGridCellClick and to trigger the modal? Then it doesn't sound like you aren't changing any props but it sounds like it still triggering the Pivot componentWillReceiveProps which wipes out / resets the current pivot.

Regardless of what you are doing exactly... I agree, the pivot shouldn't be wiped out if none of the props change. And if you change one of the Pivot props you should be shown the same pivot you were looking at before (or it should re-render according to the previous props plus the change). I like your idea of the onChange prop that gets a representation of the state and a way to pass this state back in again as a prop. I will work on a PR and see what you @GerjonM and @pat310 think.

@turnerniles
Copy link
Owner

turnerniles commented Nov 12, 2017

@GerjonM @pat310

See #103. I’m not sure I’m working towards the right solution here.

First, I added row and column props so the pivot can be rendered immediately with default columns and row. I will add filters and fix the fields appearing in the selectable fields and columns.

Secondly I added a onChange prop to the Pivot component.

If present the onChange function is executed in the Pivot componentWillReceiveProps and the normal componentWillReceiveProps resetting function is ignored.

  componentWillReceiveProps(nextProps) {
    if (nextProps.onChange !== undefined) {
      const newState = this.props.onChange(this.state);

So the state of the pivot will be updated with the newState (data will be taken from nextProps however).

So you can pass the Pivot an onChange function like so:

onChange={ (prevState) => {
     const newState = prevState;
     newState.colFields =[“name","house"];
     return newState
}}

The onChange function is called with the current state of the Pivot, passing it back to the parent component for modification, and then passes it back to the Pivot and updates the Pivot component with the next state.

With this function present, any change to the props will not call the normal componentWillReceiveProps function and allow new props such as onLeftHeaderCellClick, to be passed down without completely resetting the Pivot.

@GerjonM
Copy link
Author

GerjonM commented Nov 14, 2017

I have checked it out and this branch provides me with the options that I need. So in my opinion you are going towards the right solution.

The table can now return it's state, allowing the user to manipulate or store the configurations. And the added rows and column props allows an initial structure of the Pivot to be passed. In the future you could even consider to allow more props to be passed which would allow for a completely pre-defined Pivot.

Thank you for your work. I will continue to follow the progress.

@turnerniles
Copy link
Owner

@GerjonM Thank you for the feedback. I added filters as an additional prop to be passed to the Pivot component. I think pivots are now completely pre-definable. I've also finished having the table being able to return state. I just need to clean it up, and write some notes about the new features. I also have an issue where if you have rendered a pivot with a pivotOnChangeFunction defined, then change some props i.e. the data, with a different pivotOnChangeFunction, it will run the pivotOnChangeFunction based on the previous state of the old pivot... You can see this by switching from small dataset (initial demo render) to medium dataset (no pivotOnChangeFunction defined) then switch back to small dataset (pivotOnChangeFunction defined). I'll have to think about that a bit more but should finish this feature this weekend. If you have a chance to check it out again let me know!

@GerjonM
Copy link
Author

GerjonM commented Nov 26, 2017

The pivotOnChangeFunction does not (yet) update all of the props when changing, this causes some of the state of the previous dataset to contaminate the new one. This can become tedious with lots of data sets which all have its own initial state. I don't think it is unreasonable to reset the complete state when changing to a completely different dataset. And if the user wants some pre-defined state, he or she can pass that to the pivot.

For me, the functionality I was looking for is now present. I can play around with a pivot and save the state. And when I re-render (or change dataset), I can inject my configuration into the pivot from wherever I stored it or start with a blank slate if I have no stored configuration. Thanks a lot :)

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