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

Bug Report: DS:select is getting triggered before DS:start:pre when trying to prevent selection on click #229

Open
flazouh opened this issue Jul 26, 2024 · 1 comment

Comments

@flazouh
Copy link

flazouh commented Jul 26, 2024

Description:
It appears that the ds:start:pre event is being triggered after the ds:select event. This is contrary to the expected behavior where ds:start:pre should be called before ds:select. This issue affects the logic for initializing and handling drag selection in the provided code snippet.

I am trying to prevent the selection of an element when clicking on it directly, and only enable the selection when the selection started outside the selectable elements.

Steps to Reproduce:

  1. Implement the provided code snippet in your project.
  2. Attempt to click directly on a selectable element to select it.
  3. Attempt to start a selection outside the selectable elements and drag over them.
  4. Observe the console logs or debugging tools to monitor the order of event triggers and selection behavior.

Expected Behavior:
The ds:start:pre event should be called before the ds:select event to ensure proper initialization and handling of drag selection. Clicking directly on a selectable element should not select it, while starting the selection outside should enable selection.

Actual Behavior:
The ds:select event is being triggered before the ds:start:pre event, causing the element to be selected even when clicked directly.

Code Snippet:

import React, { useEffect, useRef } from 'react';

const DragSelectComponent = () => {
  const elementRef = useRef(null);
  const ds = useDragSelect();

  useEffect(() => {
    const element = elementRef.current;
    if (!element || !ds) return;
    ds.addSelectables(element);
  }, [ds, elementRef]);

  useEffect(() => {
    if (!ds) return;

    const startID = ds.subscribe('DS:start:pre', ({ event, isDragging }) => {
      log({ message: 'DS:start:pre triggered' });
      if (isDragging) ds.break();
      const currentTarget = event.currentTarget;
      if (currentTarget?.classList?.contains('selectable-item')) {
        ds.break();
      }
    });

    const selectID = ds.subscribe('DS:select', ({ item, isDragging }) => {
      log({ message: 'DS:select triggered' });
      const selectedItemID = item.id;
      if (isDragging) return;
      // Custom logic for handling selected item
    });

    return () => {
      ds.unsubscribe('DS:start:pre', startID);
      ds.unsubscribe('DS:select', selectID);
    };
  }, [ds]);

  return <div ref={elementRef} className="selectable-item">Selectable Element</div>;
};

export default DragSelectComponent;

import React, { createContext, useState, useEffect, useContext } from 'react'
import type { DSInputElement } from 'dragselect'
import DragSelect from 'dragselect'

// Add a new prop for the selection change callback
type ProviderProps = {
  children: React.ReactNode
  settings?: ConstructorParameters<typeof DragSelect<DSInputElement>>[0]
}

const Context = createContext<DragSelect<DSInputElement> | undefined>(undefined)

function DragSelectProvider({ children, settings }: ProviderProps) {
  const [ds, setDS] = useState<DragSelect<DSInputElement>>()

  useEffect(() => {
    setDS((prevState) => {
      if (prevState) return prevState
      return new DragSelect({})
    })

    return () => {
      if (ds) {
        ds.stop()
        setDS(undefined)
      }
    }
  }, [ds])

  useEffect(() => {
    ds?.setSettings(settings || {})
  }, [ds, settings])

  return <Context.Provider value={ds}>{children}</Context.Provider>
}

function useDragSelect() {
  return useContext(Context)
}

Additional Information:
This issue disrupts the logical flow and may cause unexpected behaviors in the drag selection functionality. Please investigate the event sequencing and ensure that ds:start:pre is called before ds:select.

Logs/Console Output:

DS:select triggered
DS:start:pre triggered

Thank you for your attention to this matter.

@ThibaultJanBeyer
Copy link
Owner

ThibaultJanBeyer commented Oct 26, 2024

Hey @flazouh, thanks a lot for using the tool and taking the time to write this message I'm looking into it.

I'm reading:

The ds:start:pre event should be called before the ds:select event to ensure proper initialization and handling of drag selection. Clicking directly on a selectable element should not select it, while starting the selection outside should enable selection.

I see this code:

…
const startID = ds.subscribe('DS:start:pre', ({ event, isDragging }) => {
      log({ message: 'DS:start:pre triggered' });
      if (isDragging) ds.break();
…

Are you trying to disable the dragging functionality? You can do that via settings: draggability: false.

…
      const currentTarget = event.currentTarget;
      if (currentTarget?.classList?.contains('selectable-item')) {
        ds.break();
      }
…

Is it that you don't want elements to be selected by click but only by dragging a selection around them?
I believe there is no easy way for now but I can add a settings for this, just like immediateDrag maybe immediateSelect would that help? 🤔

Long story short

I did look into this and the exposed PRE is supposed to run before the actual code in dragselect but not before the internal PRE code. I think this is some conception question. Changing that logic to be before the internal PRE might be a breaking change. Here is the potential PR. The order of the events seem a bit fuzzy indeed.

What you are looking for in this request is Interaction:start:pre I believe. Which is not exposed but can be used.

ds.subscribe('Interaction:start:pre', ({ event, isDragging }) => {
  console.log({ message: 'Interaction:start:pre triggered' })
})

This is the very first event that gets fired on any interaction. Would you be so kind to see if any other solution I wrote about before would be a better solution for your use-case or if Interaction:start:pre works for you.

What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants