birdwingo / react-native-swipe-modal

🚀 Swipe modal is a customizable and animated modal component that can be used in React Native applications. It provides a smooth swipe-to-close functionality along with various configuration options to suit different use cases.
https://birdwingo.com
MIT License
45 stars 1 forks source link

feat: controlled state test #23

Closed barrownicholas closed 1 month ago

barrownicholas commented 1 month ago

For our app, we wanted to move to a more modern approach with React, which is to use Controlled Components/States instead of needing to instantiate a bunch of refs, pass those refs, wait for those refs.current to not be null, and then call the methods exposed to those refs.

The thought process, here, is that it is simply much easier to pass an open state (boolean) and a setOpen callback ((open: boolean) => void) to the component. From the top level, all I need to do is call setOpen(true).

This PR is probably not ready to ship at this exact moment, but I wanted to open it up for feedback and peer review. I turned on Allow edits by maintainers and welcome edits/fixes/etc.

barrownicholas commented 1 month ago

The major change introduced, here, would allow a user to declare a modal like:

import SwipeModal from '@birdwingo/react-native-swipe-modal';
import {useState} from "react";

export const Component = () => {

  const [open, setOpen] = useState<boolean>(false);

  return (
    <SwipeModal controlled open={open} setOpen={setOpen}>
      <>{"..."}</>
    </SwipeModal>
  );

}
Lipo11 commented 1 month ago

First of all, thank you for using and maintaining our library, we greatly appreciate it! Let's take a closer look at the issue: I understand what you want to achieve, but what are the advantages?

What are the advantages of having a reference (which was the reason):

As the library is modal, the reason for using a reference is that it usually needs to run after some interaction and not immediately, so null is very uncommon. You can always use useEffect with dependency to check if exists. However, I don't see anything wrong with your solution, the only thing I would change is renaming 'controlled' to 'state'.

barrownicholas commented 1 month ago

@Lipo11 thanks for getting back to me!

However, I don't see anything wrong with your solution, the only thing I would change is renaming 'controlled' to 'state'.

The controlled boolean was for TypeScript purposes, as it allowed for a bit better discrimination between the controlled state versus the ref-dependent state. I can try and remove it if you wish, I'm just not sure how TypeScript is going to behave with it. What are your thoughts?

Lipo11 commented 1 month ago

thank you for the changes! we will update readme later, but enjoy the changes in the 1.2.1

barrownicholas commented 1 month ago

@Lipo11 many thanks to you as well!