react-cursor-position icon indicating copy to clipboard operation
react-cursor-position copied to clipboard

Switch to render prop function pattern

Open ragalie opened this issue 5 years ago • 5 comments

Hello! I'd like to use this library with TypeScript (see also https://github.com/ethanselzer/react-cursor-position/issues/26), but I was having trouble writing definitions with the current implementation.

For example, we want to make sure that PositionLabel receives the cursor props. We can enforce that by specifying that the component expects those properties:

interface DetectedEnvironment {
  isMouseDetected: boolean;
  isTouchDetected: boolean;
}

interface ElementDimensions {
  width: number;
  height: number;
}

interface Position {
  x: number;
  y: number;
}

interface Props {
  detectedEnvironment: DetectedEnvironment;
  elementDimensions: ElementDimensions;
  isActive: boolean;
  isPositionOutside: boolean;
  position: Position;
}

class PositionLabel extends React.Component<Props, State> {
  ...
}

To make this work under the current implementation, though, we need to pass in a "zero-value" template when we instantiate the component. And not just for the component that we want to pass the cursor data into, but any component nested under the ReactCursorPosition component.

<ReactCursorPosition>
  <PositionLabel detectedEnvironment={{isMouseDetected: false, isTouchDetected: false}} elementDimensions=... />
  <InstructionLabel detectedEnvironment={{isMouseDetected: false, isTouchDetected: false}} elementDimensions=... />
</ReactCursorPosition>

Instead of trying to make TypeScript definitions work under the current implementation, I'm opening this PR recommending that we switch to using the render prop function pattern in this library. This is a pattern widely used within the React community, and a particularly good fit for libraries like this one that expose data to other components. In fact, the React documentation on the render prop pattern uses a mouse tracking component as its prototypical example: https://reactjs.org/docs/render-props.html

In this implementation, the children prop to ReactCursorPosition is required to be a function that accepts as arguments the cursor props. The properties can then be passed into the relevant downstream nodes.

I suggest you look at the commits independently, as that's the easiest way to get a handle on what's proposed. These are the two that I'd look at first:

https://github.com/ethanselzer/react-cursor-position/commit/1c4b56438ca53678014db39dd22d0300b0bd33f8: Implements the render prop function pattern and updates the README https://github.com/ethanselzer/react-cursor-position/commit/3b0be5606304f3c80efc9d135c474006d15e532b: Implements type definitions

Thanks for taking a look! I look forward to your feedback.

ragalie avatar Jan 13 '19 16:01 ragalie

Coverage Status

Coverage remained the same at 100.0% when pulling 642c719746d776565c18be7e7aafcb571f0380eb on ragalie:render-prop into 0f594bb6f08e774c68ef131d883ece1a55823ad0 on ethanselzer:master.

coveralls avatar Jan 13 '19 16:01 coveralls

@ragalie - This looks amazing! What are your thoughts around retaining the previous "component decorator" pattern for backward compatibility? Consumers would have the choice of using either pattern.

ethanselzer avatar Jan 28 '19 06:01 ethanselzer

If you think the render prop pattern is a better long-term fit (I think it is!), then I'd recommend replacing the component decorator pattern with the render prop one and not making both available going forward.

If we keep both around, then we either develop for both paths (which adds time and complexity to improvements), or we only develop for the render prop path, in which case the functionality is the same between 3.0.3 and 4+ along the component decorator path.

So unless we're planning on developing for both paths in the future (which I wouldn't recommend), it simplifies things for maintainers and for consumers to maintain 3.0.x using the component decorator pattern and to release 4+ using the render prop pattern.

ragalie avatar Feb 09 '19 19:02 ragalie

Is there still no typescript definition for this lib?

dguay avatar Apr 02 '20 19:04 dguay

I'd still love to see this get merged in. @ethanselzer what do you think?

ragalie avatar May 18 '20 00:05 ragalie