Added GridGap functionality to grid component #60
Conversation
There was a problem hiding this comment.
Really nice PR! Well executed grid gap functionality. 👌
Overall, I would rename 'gridGap' for just 'gap', that seems more appropriate since we are on a grid context.
There is one other slight thing to do though, that with a small gap is not appreciable, but it occurs when the gap is big enough. When we are dragging or resizing a grid element, there is a function that converts the mouse coordinates to the correct grid x and y positions. That function should also consider the 'gap' to correctly calculate it.
These functions that do this job are located on the file 'grid.utils.ts':
function screenXPosToGridValue(screenXPos: number, cols: number, width: number): number {
return Math.round((screenXPos * cols) / width);
}
function screenYPosToGridValue(screenYPos: number, rowHeight: number, height: number): number {
return Math.round(screenYPos / rowHeight);
}Grid 'x' calculation is already okay, but not 'y'. Also, if you can rename those functions to something like 'gridXPosToGridValue' would be nice, since that 'screenXPos' is not screen position, is the mouse position relative to the grid.
If you can check it it would be great, with that, the PR is ready to go to 'main' branch.
README.md
Outdated
| @@ -94,6 +94,9 @@ Here is listed the basic API of both KtdGridComponent and KtdGridItemComponent. | |||
| /** Layout of the grid. Array of all the grid items with its 'id' and position on the grid. */ | |||
| @Input() layout: KtdGridLayout; | |||
|
|
|||
| /** Grid gap in css pixels */ | |||
| @Input() gridGap: number = 0; | |||
There was a problem hiding this comment.
What about renaming 'gridGap' for just 'gap'? We are on a 'grid' context, so 'grid' prefix is not necessary.
| ? item.x * itemWidthPerColumn | ||
| : item.x * itemWidthPerColumn + gridGap * item.x, | ||
| width: item.w * itemWidthPerColumn + gridGap * Math.max(item.w - 1, 0), | ||
| height: item.h * rowHeight + gridGap * Math.max(item.h - 1, 0), |
|
I nearly forgot, before merging it to 'main' branch, you would need to 'squash' all the commits you have made into a single one. To keep the history clean |
Devin-Harris
force-pushed
the
grid-gap-feature
branch
from
be656a3 to
97f0404
Compare
|
@llorenspujol I have gone through and renamed the gridGap to gap, updated the screenXPosToGridValue and screenYPosToGridValue methods to consume the gap and use that in its calculation, and finally squashed all the commits. Let me know if their is anything else I need 😄 |
| function screenYPosToGridValue(screenYPos: number, rowHeight: number, height: number): number { | ||
| return Math.round(screenYPos / rowHeight); | ||
| function gridYPosToGridValue(screenYPos: number, rowHeight: number, height: number, gap: number): number { | ||
| return Math.round((screenYPos + gap) / rowHeight + gap); |
There was a problem hiding this comment.
I may miss something about this solution, but I think it does not fix the issue when the gap is really big, waht about something really simple like:
return Math.round(screenYPos / (rowHeight + gap));
It is not 100% accurate, but would work for now. I am thinking in completely change this function, so for now this would be enough.
There was a problem hiding this comment.
I think I missed some parenthesis as I now see why that was added...
return Math.round((screenYPos + gap) / (rowHeight + gap))
This allows for the bottom of the placeholder to show when the gap is big relative to the rowHeight (say 100 gap to 50 rowHeight). Without this, resizing downwards forces the user to scroll past the desired row they want to resize into for the placeholder to scale correctly. Its not a huge issue, just visually seemed a bit off, but I can go with your solution if you prefer it.
| function screenXPosToGridValue(screenXPos: number, cols: number, width: number): number { | ||
| return Math.round((screenXPos * cols) / width); | ||
| function gridXPosToGridValue(screenXPos: number, cols: number, width: number, gap: number): number { | ||
| return Math.round(((screenXPos + gap) * cols) / width); |
There was a problem hiding this comment.
This function I think it was okay as it was before, no changes needed IMO.
There was a problem hiding this comment.
Similar to the gridYPosToGridValue issue, when resizing with a big gap relative to the column width its becomes very difficult to see a large portion of the placeholder, especially when resizing from a bigger width to a smaller one. You could simply drag up a bit and base your resize off the sliver of the placeholder you can see at the bottom, but I thought it felt better having the placeholder snap a bit earlier so the width changes are more apparent/visible. What are your thoughts?
| left: | ||
| item.x === 0 || item.x === cols - 1 | ||
| ? item.x * itemWidthPerColumn | ||
| : item.x * itemWidthPerColumn + gap * item.x, |
There was a problem hiding this comment.
I found a bug here, happens when I try to move an item to the last column. It does not get well calculated. Why not just set left: item.x * itemWidthPerColumn + gap * item.x? It seems that would work in all cases, is there some edge case I am missing?
There was a problem hiding this comment.
Ah yes their is an issue here. The edge case I was trying to address is the first and last column only have a gap to its right and left respectively. Since we are assigning the left property here, the check against the last column is not needed because it will still need to move over the additional gap spacing. The first column check would need to remain, but since the item.x would be zero in that case I think just the else part of that condition would cover it so I agree that should be simplified down. Will also do this for the top attribute as their is an unneeded conditional there as well.
|
Thanks for the changes @Devin-Harris . I added some other comments and a small bug I found, when resolved it would be ready to merge! |
|
Hey @Devin-Harris, I have pushed a commit with the fix for the drag and resize mouse coordinates mapping to grid coordinates, with gap support. I think the overall solution can be really simpler than it is now, but I would have to think a little bit longer to get it😅. For now, this fixes seems solid to me. Please, if you can test it and provide feedback would be nice. After validating these changes are okay, would be nice to squash all those commits into a single one in order to be merged. We are close! 😃 |
Devin-Harris
force-pushed
the
grid-gap-feature
branch
from
9dda9b4 to
d9ed6b7
Compare
|
@llorenspujol Your commit works great! Splitting the w, h and x, y calculations makes those methods much clearer to me and seems to resolve those resizing issues I was running into. I have also squashed the commits now 😄 |
|
Merged! 👏 |
|
@Devin-Harris I see that you don't show up as a contributor in the repo, this may be because the email address you used to author the commits isn't connected to your account on GitHub. If you want to appear as a contributor you would need to fix that. If you like I can undo the merge and redo afterwards with your correct account. Just tell me if you want it. |
Ah whoops. Its no big deal though. I'm happy just knowing I will be able to use this feature in the future |
Feature/Merge request for Grid Gap functionality described in the TODO features of the project.
I notice their has been a workaround listed in #43, but wanted to make a formal issue to potentially support this feature.
Change list includes the addition of a gridGap input to the grid component, logic for calculating the grid items top, left, width, and height properties accounting for this new gridGap property, and similar logic for calculating the height of the grid. I also added an input to the playground demo so this can be seen visually.
Feel free to build off this and discuss any potential issues/changes that could be beneficial here