-
Notifications
You must be signed in to change notification settings - Fork 166
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.
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
Fix/canvas dragging with margin #91
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would make this PR into just the margin controls part, and make a second PR for the actual dragging problem.
For that second PR, I would make it so that PinFrameChange and PinMoveChange etc are really just contain the mouse movement during the drag, and they'd apply that incrementally to the values that are already stored in the jsx. So instead of "dragged this to left: 150", I think the good approach would be "dragged this 5 pixels to the right", because the drag-to-move currently suffers from the big problem that when you drag something even by one pixel, we basically apply a whole new frame to the dragged element instead of just manipulating the relevant values.
scale: number | ||
} | ||
|
||
export const MarginControls = (props: MarginControlsProps) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would this benefit from betterReactMemo
?
|
||
export const MarginControls = (props: MarginControlsProps) => { | ||
if (props.margin == null) { | ||
return <></> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we return null
in most components
left: props.frame.x + props.canvasOffset.x - props.margin.left, | ||
top: props.frame.y + props.canvasOffset.y - (props.margin.top || 0), | ||
width: props.margin.left, | ||
height: props.frame.height + (props.margin.top || 0) + (props.margin.bottom || 0), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe (props.margin.top ?? 0)
is better?
) | ||
|
||
// return [leftElement, topElement, rightElement, bottomElement, outerDiv] | ||
return <>{...[leftElement, topElement, rightElement, bottomElement, outerDiv]}</> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe you don't need the spread here
/> | ||
) | ||
|
||
// return [leftElement, topElement, rightElement, bottomElement, outerDiv] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
commented out code!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, i didn't notice this
Closing this. I'm making 2 PRs from it. |
The issue is that dragging an element on the canvas that has margin set on them is jumping.
Fix is to show margin control for all elements and make dragging/resizing offset the the new frame by the top/left margin coming from the metadata.
Changed
getYogaMargin
togetElementMargin
because it is used for all elements now.Moved MarginControls out from flex canvas controls to selection outline.
Removed
hideMarginControls
from flex canvas controls, this was not used.Closes #59