improves new react demo example #2162
Conversation
|
|
||
| }) | ||
| .on('change', (ev, gsItems) => { | ||
| const newItems = grid.save(false); |
There was a problem hiding this comment.
that is not the best way to do this... it's going to set a new list and REBUILD EACH widget all over again when only a few might have changed AND really only the x,y,w,h might have changed so the structure values need to change but not trigger a react rebuild because the items are already at the correct location. Only client side structure needs syncing.
There was a problem hiding this comment.
Do you mean creating a list with new objects will force GridStack to rebuild each widget or React? Because as far as I know it is not the problem for React since the ids that are used as keys have not been changed, hence React will not remount elements. Please correct me if i am wrong.
There was a problem hiding this comment.
did you actually debug react and made sure no new elements were created ?
I know in Angualr wrapper I did it doesn't as the id have not changed (what it keys off).
Still you could just update the few elements, but save() is pretty efficient...
| } else { | ||
| // | ||
| // update existing grid layout, which is optimized to updates only diffs (will add new/delete items for examples) | ||
| // | ||
| const grid = gridRef.current; | ||
| const layout = []; | ||
| items.forEach((a) => layout.push( | ||
| const layout = items.map((a) => | ||
| refs.current[a.id].current.gridstackNode || {...a, el: refs.current[a.id].current} |
There was a problem hiding this comment.
Regarding this line, i see that you have added el property to each item, I also had to do so when i was writing a React wrapper component to avoid creating a new widget instead of updating. And i had to ts-ignore it, since i got typescript error, because the load() function expects Widget[] as a parameter. It looks like a typing error in load().
This problem forced me to dig into the sources of GridStack
There was a problem hiding this comment.
yeah, the problem is the id YOU provide is not guaranteed to be there, nor unique, so the lib cannot depend on it being there... that's why I have _id internally to know what they are. so el is used (if present, else creates a new item).
I suppose I could check if id is set and assume callee does the right thing....
if you look at the Angular wrapper, I get the gridstackNode (which has _id and .el) to load, else it uses widget for new items positions...
https://github.com/gridstack/gridstack.js/blob/master/demo/angular/src/app/gridstack.component.ts#L129
with
https://github.com/gridstack/gridstack.js/blob/master/demo/angular/src/app/gridstack-item.component.ts#L39
There was a problem hiding this comment.
I shoudl document that better... but again have 2 list is the wrong approach.
https://github.com/gridstack/gridstack.js/tree/master/demo/angular/src/app Angular wrapper is the right way.
There was a problem hiding this comment.
I have made my own react wrapper component over gridstack with a custom hook which can make any component draggable and GridStackDraggable component to make any react component gridstack draggable. Do you wish to have a look on it?
There was a problem hiding this comment.
that would be great - if you could post something similar to the Angular component I created, that would help others as I don't use React. Then peolple can interate and improved on it.
| @@ -68,45 +69,35 @@ <h2>Controlled stack</h2> | |||
| }); | |||
| }) | |||
| .on('removed', (ev, gsItems) => { | |||
| /* Looks like a bug in GridStack */ | |||
There was a problem hiding this comment.
what do you mean ? 'removed' should only be called on item being removed | added, and change on existing ones changing values. all 3 cases need to update your list - which is why I don't like having 2 list to sync and why this is not the best way...
There was a problem hiding this comment.
There is one case when you move one widget from one grid into another and when the widget is removed form the original grid, a relayout occurs but the change event is not triggered despite the fact that some widgets changed their position, so i have to manually check this and call change handler to have the relevant layout
I can try to make a video that will explain what i mean if you wish
Description