Skip to content

Conversation

@jakubbialas
Copy link

I have been happily using grid with compact mode 'none' and 'prevent collision' enabled for the last 3 years, until my PO decided he want to enable collisions. As you might know it isn't working grate in that combination (items spread across the grid and single mis-drag can destroy whole layout):

before.mp4

I implemented feature,that should change behaviour in compact mode 'none' without 'prevent collision', It basically restores original layout before resolving collisions, that way items are returning to their places if not colliding with dragged item:

after.mp4

I also made resizing items more deterministic, by extending moveElement function into more like moveAndResizeElement.

@llorenspujol please have a look at it, I'm open to any questions and feedback.

@amanosacrous
Copy link
Contributor

Thanks again for the improvements @jakubbialas !

I’m from the Katoid team, and we’ve been discussing the feature with @llorenspujol. We’ll answer you step by step.

CLONE LAYOUT
The fact of making a copy of the original layout on each iteration seems perfect to us, cause it avoids the persistence of undesired element shifts as you say.

MOVE ELEMENTS AWAY FROM COLISION
We agree that in the "no compact" type, elements are swaped once the moving element passes halfway over the one being swapped (It balances better when moving elements up and down) . And also that self-collisions should not be detected.
However, we don’t see the need to shift more than one position during recursion, since the final compact step already handles that. The idea is to minimize code differences between compact types.

RESIZING
Finally, calling moveElement() during resize seems low priority to us. Why did you decide to handle it this way? It works fine, but the fact that resizing also reorders the grid might not be desirable for everyone. And also it would add complexity to the code for handling multiple movements.

itemToMove,
compactH ? itemToMove.x + 1 : undefined,
compactV ? itemToMove.y + 1 : undefined,
compactV ? itemToMove.y + 1 : (noCompact ? Math.max(itemToMove.y + 1, collidesWith.y + 1) : undefined),
Copy link
Contributor

Choose a reason for hiding this comment

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

Better mantain compactV ? itemToMove.y + 1 : undefined, and let the compact function handle it

const newLayoutItems: LayoutItem[] = config.layout.map((item) => {
return item.id === gridItemId ? layoutItem : item;
});
const layoutItems: LayoutItem[] = config.layout;
Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid added complexity, we think it’s better to leave the resize behavior as it was.

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

Successfully merging this pull request may close these issues.

2 participants