Skip to content

improves new react demo example #2162

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Jan 14, 2023
Merged

improves new react demo example #2162

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
07f4f15
improves new react demo example
Jan 13, 2023
97c5638
adds usage of ref instead of an id property
Jan 13, 2023
1031f28
removes id property in the example
Jan 13, 2023
744e801
reverts indentation
Jan 13, 2023
d4692b0
reverts indentation
Jan 13, 2023
59deeb8
reverts indentation
Jan 13, 2023
72aaab8
minor improvements
Jan 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 54 additions & 53 deletions demo/react-hooks-controlled-multiple.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,10 @@ <h2>Controlled stack</h2>
//
// Controlled example
//
const ControlledStack = ({ items, addItem, removeItem, id }) => {
const ControlledStack = ({ items, addItem, removeItem, changeItems }) => {
const refs = useRef({})
const gridRef = useRef()
const gridContainerRef = useRef(null)
refs.current = {}

if (Object.keys(refs.current).length !== items.length) {
Expand All @@ -49,13 +50,13 @@ <h2>Controlled stack</h2>
// no need to init twice (would will return same grid) or register dup events
const grid = gridRef.current = GridStack.init(
{
float: true,
float: false,
acceptWidgets: true,
disableOneColumnMode: true, // side-by-side and fever columns to fit smaller screens
column: 6,
minRow: 1,
},
`.controlled-${id}`
gridContainerRef.current
)
.on('added', (ev, gsItems) => {
if (grid._ignoreCB) return;
Expand All @@ -68,45 +69,35 @@ <h2>Controlled stack</h2>
});
})
.on('removed', (ev, gsItems) => {
/* Looks like a bug in GridStack */
Copy link
Member

@adumesny adumesny Jan 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor Author

@ruslauz ruslauz Jan 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@adumesny adumesny Jan 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed it #2163. thanks !

const dirtyNodes = grid.engine.getDirtyNodes();
if (dirtyNodes !== undefined && dirtyNodes.length !== 0) {
const newItems = grid.save(false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@ruslauz ruslauz Jan 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@adumesny adumesny Jan 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I did.

changeItems(newItems);
}

if (grid._ignoreCB) return;
gsItems.forEach(n => removeItem(n.id));
})

.on('change', (ev, gsItems) => {
const newItems = grid.save(false);
changeItems(newItems);
})
} 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}
Copy link
Contributor Author

@ruslauz ruslauz Jan 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@adumesny adumesny Jan 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@adumesny adumesny Jan 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

));
);
grid._ignoreCB = true; // hack: ignore added/removed since we're the one doing the update
grid.load(layout);
delete grid._ignoreCB;
}

// NOTE: old code is incorrect as it re-does the GS binding, but dragged item is left behind so you get dup DOM elements with same ids
// grid.batchUpdate()
// items.forEach((a) => {
// // remove existing widgets
// if (refs.current[a.id] && refs.current[a.id].current) {
// grid.removeWidget(refs.current[a.id].current, false, false)
// }
// grid.makeWidget(refs.current[a.id].current)
// })
// grid.batchUpdate(false)

}, [items])

useEffect(() => {
return () => {
// console.log('cleanup', id)
// gridRef.current.destroy(false, false)
// gridRef.current = null
}
})

return (
// ********************
// NOTE: constructing DOM grid items in template when gridstack is also allowed editing (dragging between grids, or adding/removing from say a toolbar)
Expand All @@ -119,10 +110,10 @@ <h2>Controlled stack</h2>
// is not robust as things get added, and pollutes the DOM attr for default/missing entries, vs optimized code in GS.
// ********************
<div style={{ width: '100%', marginRight: '10px' }}>
<div className={`grid-stack controlled-${id}`}>
<div className="grid-stack" ref={gridContainerRef}>
{items.map((item, i) => {
return (
<div ref={refs.current[item.id]} key={`${id}-${item.id}`} className={'grid-stack-item'} gs-id={item.id} gs-w={item.w} gs-h={item.h} gs-x={item.x} gs-y={item.y}>
<div ref={refs.current[item.id]} key={item.id} className="grid-stack-item" gs-id={item.id} gs-w={item.w} gs-h={item.h} gs-x={item.x} gs-y={item.y}>
<div className="grid-stack-item-content">
<Item {...item} />
</div>
Expand All @@ -142,32 +133,42 @@ <h2>Controlled stack</h2>
const [items2, setItems2] = useState([{ id: 'item-2-1', x: 0, y: 0, w: 1, h: 1 }, { id: 'item-2-2', x: 0, y: 1, w: 1, h: 1 }, { id: 'item-2-3', x: 1, y: 0, w: 1, h: 1 }])

return (
<div style={{display: 'flex'}}>
<div style={{ display: 'flex', width: '50%' }}>
<ControlledStack
id='gs1'
items={items1}
addItem={(item) => {
setItems1(items => [...items, item])
}}
removeItem={(id) => {
setItems1(items => items.filter(i => i.id !== id))
}}
/>
</div >
<div style={{ display: 'flex', width: '50%' }}>
<ControlledStack
id='gs2'
items={items2}
addItem={(item) => {
setItems2(items => [...items, item])
}}
removeItem={(id) => {
setItems2(items => items.filter(i => i.id !== id))
}}
/>
<div>
<div style={{display: 'flex', gap: '16px', marginBottom: '16px'}}>
<div></div>
</div>

<div style={{ display: 'flex', gap: '16px', marginBottom: '16px' }}>
<button onClick={() => setItems1(items => [...items, { id: `item-1-${Date.now()}`, x: 2, y: 0, w: 2, h: 2 }])}>Add Item to 1 grid</button>
<button onClick={() => setItems2(items => [...items, { id: `item-2-${Date.now()}`, x: 2, y: 0, w: 2, h: 2 }])}>Add Item to 2 grid</button>
</div>
</div >
<div style={{display: 'flex'}}>
<div style={{ display: 'flex', width: '50%' }}>
<ControlledStack
items={items1}
addItem={(item) => {
setItems1(items => [...items, item])
}}
removeItem={(id) => {
setItems1(items => items.filter(i => i.id !== id))
}}
changeItems={(items) => setItems1(items)}
/>
</div >
<div style={{ display: 'flex', width: '50%' }}>
<ControlledStack
items={items2}
addItem={(item) => {
setItems2(items => [...items, item])
}}
removeItem={(id) => {
setItems2(items => items.filter(i => i.id !== id))
}}
changeItems={(items) => setItems2(items)}
/>
</div>
</div >
</div>
)
}

Expand Down