droppable dimensions include the size of the draggable placeholder if… #132
Conversation
|
I've already noticed a bug with this - if a droppable has a min-height but not enough items to fill it out the placeholder size will still be added to its dimensions unnecessarily. I'll keep working on this. |
|
Can you please explain the reasoning behind this change? If we merge #123 is this needed? |
|
Can you please also find another reviewer for this one? |
src/state/get-droppable-over.js
Outdated
| DroppableDimensionMap, | ||
| DroppableDimension, | ||
| } from '../types'; | ||
|
|
||
| export default ( | ||
| const noPadding: Position = { x: 0, y: 0 }; |
There was a problem hiding this comment.
rather than using the name padding which is already a thing, let's use another name. What about 'buffer'? or 'extra'?
noBuffer
src/state/get-droppable-over.js
Outdated
| isPointWithin(droppable)(target) | ||
| )); | ||
| .find((droppable: DroppableDimension): boolean => { | ||
| const placeholderPadding = (() => { |
There was a problem hiding this comment.
Can you add the flow type to this variable as well as to the others in this file?
placeholderBuffer: Position
| @@ -21,24 +26,37 @@ export const isPointWithin = (droppable: DroppableDimension) => { | |||
| // This gives us the droppable's true visible area | |||
| // Note: if the droppable doesn't have a scroll parent droppableBounds === container.page | |||
| const containerBounds = droppable.container.page.withoutMargin; | |||
| const clippedBounds: Spacing = { | |||
| const clippedBounds = { | |||
| export const isPointWithin = (droppable: DroppableDimension) => { | ||
| const noPadding: Position = { x: 0, y: 0 }; | ||
|
|
||
| const getVisibleBounds = ( |
There was a problem hiding this comment.
rather than adding an additional parameter here, perhaps we should create a function that add a Position or a Spacing to a DroppableDimension
src/state/reducer.js
Outdated
| @@ -107,17 +107,25 @@ const move = ({ | |||
| windowScroll: currentWindowScroll, | |||
| }; | |||
|
|
|||
| const previousDroppableOver = state.drag && state.drag.impact.destination | |||
src/state/reducer.js
Outdated
| })); | ||
|
|
||
| const drag: DragState = { | ||
| initial, | ||
| impact: newImpact, | ||
| current, | ||
| previous: { | ||
| droppableOver: previousDroppableOver, |
There was a problem hiding this comment.
can this just be the id and we keep the dimension within the state.dimension?
There was a problem hiding this comment.
Oh sorry, I just saw it was the Id. Can this be changed to droppableOverId
|
These algorithms are also used by keyboard dragging. We need to check that they also hold up for that. Those algorithms already have logic for moving into the last place of a foreign droppable |
|
hey @alexreardon so #123 makes it a bit easier to drag into the last position of a full droppable because it includes the margin in the hit-box. This PR gives us the functionality we really want - including a placeholder's size in a droppable's dimension when appropriate. Something like this is going to be necessary for implementing auto-scrolling, but I decided to pull it out into its own PR and try to get it in sooner because it improves the experience RE #122 as well. If you have a chance (and I totally understand if you don't!) I'd be keen to get your thoughts on this approach. Currently I'm calculating the droppable's dimensions in real time during the drag (in the same place where we perform the runtime clipping), but I'm also considering capturing a 'withPlaceholder' fragment while capturing dimensions. If it's possible to do it once there I think it might be more performant. Might spike this approach on a separate branch too... 🤔 |
|
The upfront withPlaceholder sounds simplier to reason about just looking at it. Then everything to do with the dimension is in the one place |
… they are a foreign list and were hovered on the previous tick
…sPointWithin from worrying about this. memoize runtime buffering logic where possible.
jaredcrowe
force-pushed
the
droppable-dimension-includes-placeholder
branch
from
0b08ed0
to
40abf03
Compare
|
Sweet! Obviously I dont have a the requisite knowledge to comprehensively review this, but I've sat down with Jared and gone through the problem and the solution. Happy to merge this and make any changes as necessary. |
|
I do not have time to review this properly so i'll lean on @lukebatchelor and @jaredcrowe. I will remove my review |
… they are a foreign list and were hovered on the previous tick
This addresses #122 by adding the placeholder size to a droppable's dimensions when it's being hovered.