express-labs / pure-react-carousel

A highly impartial suite of React components that can be assembled by the consumer to create a carousel with almost no limits on DOM structure or CSS styles. If you're tired of fighting some other developer's CSS and DOM structure, this carousel is for you.
https://express-labs.github.io/pure-react-carousel/
MIT License
1.67k stars 163 forks source link

Provide a way to handle slide click and cancel it if mouse is moving #309

Open thomasleduc opened 3 years ago

thomasleduc commented 3 years ago

Describe the feature you'd like:

Today, when I'm implement click on slide of this slider. The click is fired when my users slide left or right the carousel. I would like the Slide onClick to be cancel if the user is sliding or best, I would like a way to know when my user is sliding in my child component.

Suggested implementation:

Put what I understand is the var I'm looking for isBeingMouseDragged (in Slider.jsx) in the Context Provider so I can use it in my component with useContext() :

import { SliderContext } from 'pure-react-carousel';

const InnerSlide : React.FC<{ id: string }> = ({ id }) => {
  const { isUserMouseMoving } = useContext(SliderContext);

  const handleClick = useCallback((e) => {
    if (isUserMouseMoving) { e.preventDefault(); return; }

    // handle the click with props id
  }, [isUserMouseMoving, id]); 

  return <div className="inner" onClick={handleClick}> my super inner slide {id}</div>;
};

Describe alternatives you've considered:

Add a listener on mouse move, mouse down, mouse up on the whole slider and do it myself. I found it sad as I think it is already done in the pure-react-carousel code.

tim-steele commented 3 years ago

Thanks @thomasleduc - so to understand you just want to know when the carousel is in the transition state, i.e., sliding?

I do think it would make sense for the context, as it contains other information around the carousel. Is this something you are open to submitting a pull request for?

thomasleduc commented 3 years ago

It's in my todo list. But I'm not sure I have an elegant solution :

  const [isMouseDown, setIsMouseDown] = useState(false);
  const [isMouseDragging, setIsMouseDragging] = useState(false);
  const timeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null);

  const handleMouseDown = useCallback(() => setIsMouseDown(true), []);

  const handleMouseUp = useCallback(() => {
    setIsMouseDown(false);
    timeoutRef.current = setTimeout(() => setIsMouseDragging(false), 0);
  }, []);

  const handleMouseMove = useCallback(
    () => isMouseDown && setIsMouseDragging(true),
    [isMouseDown]
  );

  // component unmount
  useEffect(
    () => () => {
      timeoutRef?.current && clearTimeout(timeoutRef.current);
    },
    []
  );

  return (<PureReactCarouselProvider>
      <div
        onMouseDown={handleMouseDown}
        onMouseUp={handleMouseUp}
        onMouseMove={handleMouseMove}
      >
      { /* ... */ }
      </div>
  </PureReactCarouselProvider>);

The thing is, PureReactCarouselProvider has no access to its children (could be a fragment or a non display:block element). I don't know how to get a div that cover the entire slider. Plus, I can't use document as there could be many sliders in it.

mrbinky3000 commented 3 years ago

I actually was going to tackle this during "code freeze" at Express. But hey, if you have an angle. I would say that I would not use timeouts. Perhaps we can move the onclick handlers for the slides up to the slider and capture them on bubble. That way we can cancel them if the slider is sliding?

In a nutshell, thank you very much for taking a stab at this but anything that relies on timers will be a hard pass for my vote. I'd be up for refactoring to correct this oversight. Even a breaking change, but I'm not too hip on timers for anything. It's a sign that there is a fundamental issue and in this case, you did find an issue.

On Tue, Nov 3, 2020 at 5:10 AM Thomas LEDUC notifications@github.com wrote:

It's in my todo list. But I'm not sure I have an elegant solution :

const [isMouseDown, setIsMouseDown] = useState(false); const [isMouseDragging, setIsMouseDragging] = useState(false); const timeoutRef = useRef<ReturnType | null>(null);

const handleMouseDown = useCallback(() => setIsMouseDown(true), []);

const handleMouseUp = useCallback(() => { setIsMouseDown(false); timeoutRef.current = setTimeout(() => setIsMouseDragging(false), 0); }, []);

const handleMouseMove = useCallback( () => isMouseDown && setIsMouseDragging(true), [isMouseDown] );

// component unmount useEffect( () => () => { timeoutRef?.current && clearTimeout(timeoutRef.current); }, [] ); return (

{ /* ... */ }
); The thing is, PureReactCarouselProvider has no access to its children (could be a fragment or a non display:block element). I don't know how to get a div that cover the entire slider. Plus, I can't use document as there could be many sliders in it. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub , or unsubscribe .
stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

thomasleduc commented 3 years ago

Totally agree @mrbinky3000. It was a quick launch, and to be honest we have it on the backlog but did not find a correct way to refactor it. Put the click on the slider would certainly work, even if it is really a shame to have to retrieve the slide clicked and compute a special function for it.

azurekca commented 3 years ago

Hi, is this issue still being investigated? I am having problems with this as well. I'm using a carousel to display a bunch of products and clicking on a product takes me to the product description. However, I don't want to navigate away from the page if the carousel is being scrolled.

I see in this commit that stopPropagation was taken out of handleOnClickCapture in Slider.jsx. How about adding another Slider prop to ask developers if they would like to stop event propagation?