nuclearpasta / react-native-drax

A drag-and-drop system for React Native
MIT License
555 stars 68 forks source link

Provide viewState/grabOffset in event data? #17

Closed lafiosca closed 4 years ago

lafiosca commented 4 years ago

The content rendering functions that can be passed to DraxView receive a DraxViewState prop which contains useful information like grabOffset and dragOffset: https://github.com/nuclearpasta/react-native-drax/blob/08a4414bbc33224b00cda0c4b0544680ad2e5043/src/types.ts#L379-L387

These values are not currently provided in any of the drag event data passed to the drag-drop lifecycle callbacks. Should they be? I think it would be nice in some cases to have that data available.

And this raises a more general question: would adding required fields to the shape of an interface like DraxEventData be considered a breaking change? I think technically it would be. For example, if someone is using that interface to construct objects in their own code, that could break, although the average consumer of this library would presumably only be receiving/handling such objects from the library. The interfaces/types are exposed primarily for flexibility and convenience. Still, it doesn't hurt for us to release additional minor version bumps while in 0.x.

lafiosca commented 4 years ago

Let's add the relevant values from that object to each of the dragged and receiver items in the drag events, as required rather than optional, for accuracy/convenience.

The dragged item will now have:

    /** If being dragged or released, the relative offset of the drag point from the view */
    dragOffset: Animated.ValueXY;

    /** If being dragged, the relative offset of where the view was grabbed */
    grabOffset: Position;
    /** If being dragged, the relative offset/dimensions ratio of where the view was grabbed */
    grabOffsetRatio: Position;

    /** The position in screen coordinates of the dragged hover view (dragScreenPosition - grabOffset) */
    hoverPosition: Animated.ValueXY;

Note that we omit dragScreenPosition because it's already in the event data. Also I am uncertain on hoverPosition, although I don't think it hurts to include it, and even if noHover is enabled the value still exists unused.

We've already added the relevant values to the receiver data in #15

lafiosca commented 4 years ago

I forgot that we can't access the viewState directly in the provider handler methods because we'll end up in a circular dependency. These methods only operate on the registry ref values and update state via dispatch, never depending on reading state. Still, the registry is able to calculate the updated values to dispatch to the updater, so it must be possible to have this information. It just might be awkward with the current logic organization.

lafiosca commented 4 years ago

But I completely misremembered where this data lives. Yes, it's reflected in the viewState, but some or all of it is also in the registry and accessible (perhaps with some extra convenience methods).

lafiosca commented 4 years ago

This is definitely a breaking change, as some field names will be changed for consistency as well.

lafiosca commented 4 years ago

Why are we using Animated.ValueXY for these in the first place? It's harder to access the x/y values from those. We should only be using Animated values when we need to animate them. Currently the only one the library is animating is hoverPosition, which makes sense. (DraxList also uses its own animated shifts internally, but those don't matter for this.)

lafiosca commented 4 years ago

Forgot to close this issue when it was addressed by PR #18 and released in 0.5.0