Improving box model #471
Improving box model #471
Conversation
src/state/action-creators.js
Outdated
| movement: impact.movement, | ||
| draggable, | ||
| draggables: state.dimension.draggable, | ||
| destination, | ||
| }); | ||
|
|
||
| const clientOffset: Position = subtract(newCenter, draggable.client.marginBox.center); | ||
| // TODO: WHAT!@? |
| @@ -344,14 +344,15 @@ export const drop = () => | |||
| reason: 'DROP', | |||
| }; | |||
|
|
|||
| const newCenter: Position = getNewHomeClientCenter({ | |||
| const newBorderBoxCenter: Position = getNewHomeClientBorderBoxCenter({ | |||
There was a problem hiding this comment.
more explicit naming all over the place
| @@ -1,245 +0,0 @@ | |||
| // @flow | |||
There was a problem hiding this comment.
gone. Now done mostly by css-box-model as well as droppable-dimension.js
| @@ -54,25 +56,25 @@ export default ({ | |||
| return false; | |||
| } | |||
|
|
|||
| const area: Area = child.page.borderBox; | |||
| const borderBox: Rect = child.page.borderBox; | |||
There was a problem hiding this comment.
super clear now
| } | ||
|
|
||
| // All the draggables in the destination (even the ones that haven't moved) | ||
| const draggablesInDestination: DraggableDimension[] = getDraggablesInsideDroppable( | ||
| destination, draggables | ||
| ); | ||
|
|
||
| // const data = (() => { |
| @@ -15,7 +15,7 @@ import type { | |||
|
|
|||
| type Args = {| | |||
| amount: Position, | |||
| pageCenter: Position, | |||
| pageBorderBoxCenter: Position, | |||
There was a problem hiding this comment.
much clearer naming
| @@ -20,34 +17,13 @@ export const expandByPosition = (spacing: Spacing, position: Position): Spacing | |||
| bottom: spacing.bottom + position.y, | |||
| }); | |||
|
|
|||
| export const expandBySpacing = (spacing1: Spacing, spacing2: Spacing): Spacing => ({ | |||
There was a problem hiding this comment.
No longer needed. This is done in css-box-model
|
This is in preparation for #461. I have not fixed that issue in this PR, but we really needed to improve our internal box model usage (again) to clearly resolve the issue |
| @@ -1,245 +0,0 @@ | |||
| // @flow | |||
| @@ -11,7 +11,7 @@ import type { | |||
| export type Args = {| | |||
| isMovingForward: boolean, | |||
| draggableId: DraggableId, | |||
| previousPageCenter: Position, | |||
| previousPageBorderBoxCenter: Position, | |||
There was a problem hiding this comment.
Can we be importing Position from css-box-model here (and anywhere else Position is imported)?
src/types.js
Outdated
| |} | ||
| // For simplicity exporting these from here as they are used | ||
| // very frequently within the code | ||
| export type Position = PositionType; |
There was a problem hiding this comment.
Let's be consistent about whether we remove these types and import directly from css-box-model wherever needed, or whether we keep these and make sure everything imports from here. I've noticed both approaches throughout the code 🙂
There was a problem hiding this comment.
This was helpful in my conversion - but i agree that it should be removed
| import PropTypes from 'prop-types'; | ||
| import memoizeOne from 'memoize-one'; | ||
| import invariant from 'tiny-invariant'; | ||
| import { calculateBox, withScroll } from 'css-box-model'; | ||
| import type { BoxModel } from 'css-box-model'; |
There was a problem hiding this comment.
can we consolidate these lines?
| const { borderBox, margin, display, tagName } = placeholder; | ||
| const client: BoxModel = placeholder.client; | ||
| const display: string = placeholder.display; | ||
| const tagName: string = placeholder.tagName; |
There was a problem hiding this comment.
I wanted to give the types explicitly - especially for the client: BoxModel so that it is clear
No description provided.