Skip to content
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

Make props in react / vue packages reactive #201

Closed
enawork opened this issue May 16, 2023 · 2 comments
Closed

Make props in react / vue packages reactive #201

enawork opened this issue May 16, 2023 · 2 comments
Assignees
Labels
bug Something isn't working @viselect/preact Preact package @viselect/react React package @viselect/vue Vue package

Comments

@enawork
Copy link

enawork commented May 16, 2023

Is your feature request related to a problem? Please describe.

Yes, the issue is related to the event handlers (onBeforeStart, onBeforeDrag, onStart, onMove, onStop) within the SelectionArea component not having access to the latest props when the parent component's props change. As a result, the event handlers only reference the props at the initial mount of the component.

Describe the solution you'd like

An approach to solve this issue is to include the event handlers in the dependency array of the useEffect that sets up the event handlers. This ensures that the useEffect would run every time any of the event handlers changes, causing the event listeners to be re-set with the latest props.

Example:

useEffect(() => {  
  ...  
}, [onBeforeStart, onBeforeDrag, onStart, onMove, onStop]);  

Describe alternatives you've considered

An alternative solution is to wrap the event handler functions with useRef inside the SelectionArea component. This will allow the handlers to reference the latest props whenever they are executed without the need to re-run the useEffect. Here's an example for onStart:

const onStartRef = useRef<SelectionEvents['start'] | undefined>(null);  
  
useEffect(() => {  
  onStartRef.current = onStart;  
});  
  
// Then, inside the useEffect where the event handlers are set up:  
onStart && selection.on('start', (event) => {  
  if (onStartRef.current) {  
    onStartRef.current(event);  
  }  
});  

Additional context

The current implementation may cause unexpected behavior for users who expect the event handlers to always reference the latest props. By implementing the suggested solution, the component will provide a more consistent and expected behavior.
Please note that this issue and the proposed solutions were created by an AI and have not been tested. It is recommended to test the suggested changes before implementing them in the actual codebase.

@enawork enawork added the feature request New feature requested label May 16, 2023
@enawork
Copy link
Author

enawork commented May 18, 2023

After some trial and error, I managed to create the following React wrapper for my project, and by importing @viselect/vanilla, I was able to workaround the issue mentioned above.

SelectionArea.js

/* eslint-disable no-use-before-define */
import VanillaSelectionArea from '@viselect/vanilla'
import React from 'react'

export default function SelectionArea (props) {
  const { onBeforeStart, onBeforeDrag, onStart, onMove, onStop, ...opt } = props
  const root = React.createRef()

  const onBeforeStartRef = React.useRef()
  const onBeforeDragRef = React.useRef()
  const onStartRef = React.useRef()
  const onMoveRef = React.useRef()
  const onStopRef = React.useRef()

  React.useEffect(() => {
    onBeforeStartRef.current = onBeforeStart
    onBeforeDragRef.current = onBeforeDrag
    onStartRef.current = onStart
    onMoveRef.current = onMove
    onStopRef.current = onStop
  })

  React.useEffect(() => {
    const areaBoundaries = root.current

    const selection = new VanillaSelectionArea({
      boundaries: areaBoundaries,
      ...opt
    })

    onBeforeStart && selection.on('beforeStart', (event) => {
      if (onBeforeStartRef.current) {
        onBeforeStartRef.current(event)
      }
    })
    onBeforeDrag && selection.on('beforedrag', (event) => {
      if (onBeforeDragRef.current) {
        onBeforeDragRef.current(event)
      }
    })
    onStart && selection.on('start', (event) => {
      if (onStartRef.current) {
        onStartRef.current(event)
      }
    })
    onMove && selection.on('move', (event) => {
      if (onMoveRef.current) {
        onMoveRef.current(event)
      }
    })
    onStop && selection.on('stop', (event) => {
      if (onStopRef.current) {
        onStopRef.current(event)
      }
    })

    return () => selection.destroy()
  }, [])

  return (
    React.createElement('div', { ref: root, className: props.className, id: props.id }, props.children)
  )
}

I believe it would look like this in TypeScript:
https://gist.github.com/enawork/84c0d88fb2e34ed232dbeeb603fedd9c#file-selectionarea-tsx

@simonwep
Copy link
Owner

Thanks for the elaborative feature request, I'll label this as a bug since I would expect this to be reactive either way... I'll take a look at it :)

@simonwep simonwep added bug Something isn't working @viselect/react React package @viselect/preact Preact package and removed feature request New feature requested labels Jul 21, 2023
@simonwep simonwep changed the title Update React event handlers in SelectionArea component to reference latest props Make props in react / vue packages reactive Jul 21, 2023
@simonwep simonwep added the @viselect/vue Vue package label Jul 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working @viselect/preact Preact package @viselect/react React package @viselect/vue Vue package
Projects
None yet
Development

No branches or pull requests

2 participants