Make DataTable vertically resizeable #700
Conversation
|
Your Render PR Server URL is https://toolpad-pr-700.onrender.com. Follow its progress at https://dashboard.render.com/web/srv-cbgotgnh8vl72hjrivrg. |
oliviertassinari
requested a deployment
to
vertical-element-resizing - toolpad-db PR #700
Render
Abandoned
| 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) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!
| @@ -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) { | |||
There was a problem hiding this comment.
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? 🤔
There was a problem hiding this comment.
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!
|
Nice 👌 |
resizableHeightPropScreen.Recording.2022-07-28.at.18.36.20.mov
(cursor while dragging is wrong in the video but fixed already)