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.

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

Make DataTable vertically resizeable #700

Merged
merged 2 commits into from
Aug 1, 2022
Merged

Make DataTable vertically resizeable #700

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1539ecb
Make DataTable vertically resizeable
apedroferreira Jul 27, 2022
8c678cb
Show curosr for vertical resizing
apedroferreira Jul 28, 2022
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
13 changes: 12 additions & 1 deletion packages/toolpad-app/src/toolpad/AppEditor/PageEditor/NodeHud.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
absolutePositionCss,
Rectangle,
RectangleEdge,
RECTANGLE_EDGE_BOTTOM,
RECTANGLE_EDGE_LEFT,
RECTANGLE_EDGE_RIGHT,
} from '../../../utils/geometry';
Expand Down Expand Up @@ -67,6 +68,7 @@ const DraggableEdge = styled('div', {
let dynamicStyles = {};
if (edge === RECTANGLE_EDGE_RIGHT) {
dynamicStyles = {
cursor: 'ew-resize',
top: 0,
right: -2,
height: '100%',
Expand All @@ -75,16 +77,25 @@ const DraggableEdge = styled('div', {
}
if (edge === RECTANGLE_EDGE_LEFT) {
dynamicStyles = {
cursor: 'ew-resize',
top: 0,
left: -2,
height: '100%',
width: 12,
};
}
if (edge === RECTANGLE_EDGE_BOTTOM) {
dynamicStyles = {
cursor: 'ns-resize',
bottom: -2,
height: 12,
left: 0,
width: '100%',
};
}

return {
...dynamicStyles,
cursor: 'ew-resize',
position: 'absolute',
pointerEvents: 'initial',
zIndex: 1,
Expand Down
225 changes: 136 additions & 89 deletions packages/toolpad-app/src/toolpad/AppEditor/PageEditor/RenderPanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,13 @@ import { OverlayGrid, OverlayGridHandle } from './OverlayGrid';
import NodeHud from './NodeHud';
import NodeDropArea from './NodeDropArea';

const RESIZE_SNAP_UNITS = 4; // px
const HORIZONTAL_RESIZE_SNAP_UNITS = 4; // px
const SNAP_TO_GRID_COLUMN_MARGIN = 10; // px

const VERTICAL_RESIZE_SNAP_UNITS = 2; // px

const MIN_RESIZABLE_ELEMENT_HEIGHT = 100; // px

const classes = {
view: 'Toolpad_View',
};
Expand All @@ -65,7 +69,8 @@ const overlayClasses = {
nodeHud: 'Toolpad_NodeHud',
container: 'Toolpad_Container',
componentDragging: 'Toolpad_ComponentDragging',
resize: 'Toolpad_Resize',
resizeHorizontal: 'Toolpad_ResizeHorizontal',
resizeVertical: 'Toolpad_ResizeVertical',
hudOverlay: 'Toolpad_HudOverlay',
};

Expand All @@ -79,9 +84,12 @@ const OverlayRoot = styled('div')({
[`&.${overlayClasses.componentDragging}`]: {
cursor: 'copy',
},
[`&.${overlayClasses.resize}`]: {
[`&.${overlayClasses.resizeHorizontal}`]: {
cursor: 'ew-resize',
},
[`&.${overlayClasses.resizeVertical}`]: {
cursor: 'ns-resize',
},
[`.${overlayClasses.hudOverlay}`]: {
position: 'absolute',
inset: '0 0 0 0',
Expand Down Expand Up @@ -1318,43 +1326,59 @@ export default function RenderPanel({ className }: RenderPanelProps) {
const cursorPos = canvasHostRef.current?.getViewCoordinates(event.clientX, event.clientY);

if (draggedNodeRect && parentRect && resizePreviewElement && cursorPos) {
let snappedToGridCursorPosX =
Math.round(cursorPos.x / RESIZE_SNAP_UNITS) * RESIZE_SNAP_UNITS;
if (draggedEdge === RECTANGLE_EDGE_LEFT || draggedEdge === RECTANGLE_EDGE_RIGHT) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that there is a lot of this "resizing" logic going on in this component. Could we maybe abstract it away to some ResizeablePanel component? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

in addition to the above, i haven't considered this approach so will see if there's a way to move this logic into a separate React component. thanks for the suggestion!

let snappedToGridCursorRelativePosX =
Math.ceil((cursorPos.x - draggedNodeRect.x) / HORIZONTAL_RESIZE_SNAP_UNITS) *
HORIZONTAL_RESIZE_SNAP_UNITS;

const activeSnapGridColumnEdges =
draggedEdge === RECTANGLE_EDGE_LEFT
? overlayGridRef.current.getLeftColumnEdges()
: overlayGridRef.current.getRightColumnEdges();

for (const gridColumnEdge of activeSnapGridColumnEdges) {
if (Math.abs(gridColumnEdge - cursorPos.x) <= SNAP_TO_GRID_COLUMN_MARGIN) {
snappedToGridCursorRelativePosX = gridColumnEdge - draggedNodeRect.x;
}
}

const minGridColumnWidth = overlayGridRef.current.getMinColumnWidth();

const activeSnapGridColumnEdges =
draggedEdge === RECTANGLE_EDGE_LEFT
? overlayGridRef.current.getLeftColumnEdges()
: overlayGridRef.current.getRightColumnEdges();
if (
draggedEdge === RECTANGLE_EDGE_LEFT &&
cursorPos.x > parentRect.x + minGridColumnWidth &&
cursorPos.x < draggedNodeRect.x + draggedNodeRect.width - minGridColumnWidth
) {
const updatedTransformScale =
1 - snappedToGridCursorRelativePosX / draggedNodeRect.width;

for (const gridColumnEdge of activeSnapGridColumnEdges) {
if (Math.abs(gridColumnEdge - cursorPos.x) <= SNAP_TO_GRID_COLUMN_MARGIN) {
snappedToGridCursorPosX = gridColumnEdge;
resizePreviewElement.style.transformOrigin = '100% 50%';
resizePreviewElement.style.transform = `scaleX(${updatedTransformScale})`;
}
if (
draggedEdge === RECTANGLE_EDGE_RIGHT &&
cursorPos.x > draggedNodeRect.x + minGridColumnWidth &&
cursorPos.x < parentRect.x + parentRect.width - minGridColumnWidth
) {
const updatedTransformScale = snappedToGridCursorRelativePosX / draggedNodeRect.width;

resizePreviewElement.style.transformOrigin = '0 50%';
resizePreviewElement.style.transform = `scaleX(${updatedTransformScale})`;
}
}

const minGridColumnWidth = overlayGridRef.current.getMinColumnWidth();

if (
draggedEdge === RECTANGLE_EDGE_LEFT &&
cursorPos.x > parentRect.x + minGridColumnWidth &&
cursorPos.x < draggedNodeRect.x + draggedNodeRect.width - minGridColumnWidth
draggedEdge === RECTANGLE_EDGE_BOTTOM &&
cursorPos.y > draggedNodeRect.y + MIN_RESIZABLE_ELEMENT_HEIGHT
) {
const updatedTransformScale =
1 + (draggedNodeRect.x - snappedToGridCursorPosX) / draggedNodeRect.width;
const snappedToGridCursorRelativePosY =
Math.ceil((cursorPos.y - draggedNodeRect.y) / VERTICAL_RESIZE_SNAP_UNITS) *
VERTICAL_RESIZE_SNAP_UNITS;

resizePreviewElement.style.transformOrigin = '100% 50%';
resizePreviewElement.style.transform = `scale(${updatedTransformScale}, 1)`;
}
if (
draggedEdge === RECTANGLE_EDGE_RIGHT &&
cursorPos.x > draggedNodeRect.x + minGridColumnWidth &&
cursorPos.x < parentRect.x + parentRect.width - minGridColumnWidth
) {
const updatedTransformScale =
(snappedToGridCursorPosX - draggedNodeRect.x) / draggedNodeRect.width;
const updatedTransformScale = snappedToGridCursorRelativePosY / draggedNodeRect.height;

resizePreviewElement.style.transformOrigin = '0 50%';
resizePreviewElement.style.transform = `scale(${updatedTransformScale}, 1)`;
resizePreviewElement.style.transformOrigin = '50% 0';
resizePreviewElement.style.transform = `scaleY(${updatedTransformScale})`;
}
}
},
Expand Down Expand Up @@ -1384,74 +1408,89 @@ export default function RenderPanel({ className }: RenderPanelProps) {

const parent = appDom.getParent(dom, draggedNode);

const parentChildren = parent ? appDom.getChildNodes(dom, parent).children : [];
const totalLayoutColumnSizes = parentChildren.reduce(
(acc, child) => acc + (nodesInfo[child.id]?.rect?.width || 0),
0,
);

const resizePreviewRect = resizePreviewElement?.getBoundingClientRect();

if (draggedNodeRect && resizePreviewRect) {
const normalizeColumnSize = (size: number) =>
Math.max(0, size * parentChildren.length) / totalLayoutColumnSizes;
if (draggedEdge === RECTANGLE_EDGE_LEFT || draggedEdge === RECTANGLE_EDGE_RIGHT) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest to brainstorm how this code could be made flatter, it's cyclomatic complexity seems to keep growing. In addition we should also consider how to split this file into smaller chuncks as +1.5k LoC doesn't seem to be very scalable IMO

Copy link
Member

Choose a reason for hiding this comment

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

With the risk of turning this in a religious debate, I don't like using LoC as a metric for maintainability. I've seen people trash projects by splitting code that semantically belongs together purely for the sake of reducing LoC per file.
I have no problem with 5k LoC in a file, nor do I have problems with having 100 files in a folder. As long as code is semantically grouped together, and the principle of single responsibility is respected.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not using LoC as a metric, but at certain number I'd say it signals that it could be split and that it might be doing too many things. Without spending too much time in the code my initial impressions that the code inside this component should be mostly responsible for rendering panel. This most recent addition takes care of resizing - so that might be considered as another responsibility, then skimming through the code I see mentions of:

  • dragging/dropping
  • selecting
  • node management with deletion, etc..
  • key handling
  • runtime events handling
    all of which IMO could be considered as a separate responsibility.

I'm fine with you keeping the structure as it is if you don't find it more difficult to navigate and deal with the code.

It would be just interesting to understand what would be a good examples of:

  • semantic grouping
  • principles of single responsibility
    So that it's easier to know when is the right time to consider splitting/extracting things out?

Copy link
Member Author

Choose a reason for hiding this comment

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

hey Vytautas, I understand your points and both points of view on the subject.

there is logic here that could be separated in different files according to its different responsibilities, but we would have to decide on which way to go about that:

  • i've previously suggested a non-reusable hook, but that also has it's own disadvantages and might be a misleading pattern?
  • i could try to move much of this logic into functions in separate files, but there could be obstacles in passing all the arguments we need to those functions without making them bloated - however there might be a way to make it be clean enough with some effort

Copy link
Contributor

Choose a reason for hiding this comment

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

For now please disregard my suggestion and feel free to proceed with merging when you feel comfortable. I think we could have a separate discussion where we discuss these sort of things, until we have a clear path and agreement we should not block development :)

Copy link
Member Author

@apedroferreira apedroferreira Jul 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

in regards to the many nested if statements, i might at least be able to make things flatter by just stopping the function execution when some of them are false, by returning or so - might be more doable in some cases than others but i can always make an extra effort with that in mind

Copy link
Member Author

Choose a reason for hiding this comment

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

For now please disregard my suggestion and feel free to proceed with merging when you feel comfortable. I think we could have a separate discussion where we discuss these sort of things, until we have a clear path and agreement we should not block development :)

ok, i have it noted to try to improve and make this component more concise and readable whenever i have the chance - but if we can set some common practices and standards that might be helpful!

const parentChildren = parent ? appDom.getChildNodes(dom, parent).children : [];
const totalLayoutColumnSizes = parentChildren.reduce(
(acc, child) => acc + (nodesInfo[child.id]?.rect?.width || 0),
0,
);

if (draggedEdge === RECTANGLE_EDGE_LEFT) {
const previousSibling = appDom.getSiblingBeforeNode(dom, draggedNode, 'children');
const normalizeColumnSize = (size: number) =>
Math.max(0, size * parentChildren.length) / totalLayoutColumnSizes;

if (previousSibling) {
const previousSiblingInfo = nodesInfo[previousSibling.id];
const previousSiblingRect = previousSiblingInfo?.rect;
if (draggedEdge === RECTANGLE_EDGE_LEFT) {
const previousSibling = appDom.getSiblingBeforeNode(dom, draggedNode, 'children');

if (previousSiblingRect) {
const updatedDraggedNodeColumnSize = normalizeColumnSize(resizePreviewRect.width);
const updatedPreviousSiblingColumnSize = normalizeColumnSize(
previousSiblingRect.width - (resizePreviewRect.width - draggedNodeRect.width),
);
if (previousSibling) {
const previousSiblingInfo = nodesInfo[previousSibling.id];
const previousSiblingRect = previousSiblingInfo?.rect;

domApi.setNodeNamespacedProp(
draggedNode,
'layout',
'columnSize',
appDom.createConst(updatedDraggedNodeColumnSize),
);
domApi.setNodeNamespacedProp(
previousSibling,
'layout',
'columnSize',
appDom.createConst(updatedPreviousSiblingColumnSize),
);
if (previousSiblingRect) {
const updatedDraggedNodeColumnSize = normalizeColumnSize(resizePreviewRect.width);
const updatedPreviousSiblingColumnSize = normalizeColumnSize(
previousSiblingRect.width - (resizePreviewRect.width - draggedNodeRect.width),
);

domApi.setNodeNamespacedProp(
draggedNode,
'layout',
'columnSize',
appDom.createConst(updatedDraggedNodeColumnSize),
);
domApi.setNodeNamespacedProp(
previousSibling,
'layout',
'columnSize',
appDom.createConst(updatedPreviousSiblingColumnSize),
);
}
}
}
}
if (draggedEdge === RECTANGLE_EDGE_RIGHT) {
const nextSibling = appDom.getSiblingAfterNode(dom, draggedNode, 'children');
if (draggedEdge === RECTANGLE_EDGE_RIGHT) {
const nextSibling = appDom.getSiblingAfterNode(dom, draggedNode, 'children');

if (nextSibling) {
const nextSiblingInfo = nodesInfo[nextSibling.id];
const nextSiblingRect = nextSiblingInfo?.rect;
if (nextSibling) {
const nextSiblingInfo = nodesInfo[nextSibling.id];
const nextSiblingRect = nextSiblingInfo?.rect;

if (nextSiblingRect) {
const updatedDraggedNodeColumnSize = normalizeColumnSize(resizePreviewRect.width);
const updatedNextSiblingColumnSize = normalizeColumnSize(
nextSiblingRect.width - (resizePreviewRect.width - draggedNodeRect.width),
);
if (nextSiblingRect) {
const updatedDraggedNodeColumnSize = normalizeColumnSize(resizePreviewRect.width);
const updatedNextSiblingColumnSize = normalizeColumnSize(
nextSiblingRect.width - (resizePreviewRect.width - draggedNodeRect.width),
);

domApi.setNodeNamespacedProp(
draggedNode,
'layout',
'columnSize',
appDom.createConst(updatedDraggedNodeColumnSize),
);
domApi.setNodeNamespacedProp(
nextSibling,
'layout',
'columnSize',
appDom.createConst(updatedNextSiblingColumnSize),
);
domApi.setNodeNamespacedProp(
draggedNode,
'layout',
'columnSize',
appDom.createConst(updatedDraggedNodeColumnSize),
);
domApi.setNodeNamespacedProp(
nextSibling,
'layout',
'columnSize',
appDom.createConst(updatedNextSiblingColumnSize),
);
}
}
}
}

if (draggedEdge === RECTANGLE_EDGE_BOTTOM) {
const resizableHeightProp = draggedNodeInfo?.componentConfig?.resizableHeightProp;

if (resizableHeightProp) {
domApi.setNodeNamespacedProp(
draggedNode,
'props',
resizableHeightProp,
appDom.createConst(resizePreviewRect.height),
);
}
}
}

api.dragEnd();
Expand All @@ -1473,7 +1512,10 @@ export default function RenderPanel({ className }: RenderPanelProps) {
<OverlayRoot
className={clsx({
[overlayClasses.componentDragging]: isDraggingOver,
[overlayClasses.resize]: draggedEdge,
[overlayClasses.resizeHorizontal]:
draggedEdge === RECTANGLE_EDGE_LEFT || draggedEdge === RECTANGLE_EDGE_RIGHT,
[overlayClasses.resizeVertical]:
draggedEdge === RECTANGLE_EDGE_TOP || draggedEdge === RECTANGLE_EDGE_BOTTOM,
})}
// Need this to be able to capture key events
tabIndex={0}
Expand Down Expand Up @@ -1527,6 +1569,8 @@ export default function RenderPanel({ className }: RenderPanelProps) {
? parentSlotChildNodes[parentSlotChildNodes.length - 1].id === node.id
: false;

const isVerticallyResizable = Boolean(nodeInfo?.componentConfig?.resizableHeightProp);

const nodeRect = nodeInfo?.rect || null;
const hasNodeOverlay = isPageNode || appDom.isElement(node);

Expand All @@ -1543,15 +1587,18 @@ export default function RenderPanel({ className }: RenderPanelProps) {
selected={selectedNode?.id === node.id}
allowInteraction={nodesWithInteraction.has(node.id) && !draggedEdge}
onNodeDragStart={handleNodeDragStart(node)}
draggableEdges={
isPageRowChild
draggableEdges={[
...(isPageRowChild
? [
...(isFirstChild ? [] : [RECTANGLE_EDGE_LEFT as RectangleEdge]),
...(isLastChild ? [] : [RECTANGLE_EDGE_RIGHT as RectangleEdge]),
]
: []
: []),
...(isVerticallyResizable ? [RECTANGLE_EDGE_BOTTOM as RectangleEdge] : []),
]}
onEdgeDragStart={
isPageRowChild || isVerticallyResizable ? handleEdgeDragStart : undefined
}
onEdgeDragStart={isPageRowChild ? handleEdgeDragStart : undefined}
onDelete={handleDelete(node.id)}
isResizing={Boolean(draggedEdge) && node.id === draggedNodeId}
resizePreviewElementRef={resizePreviewElementRef}
Expand Down
Loading