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

use of dialog tag for modal structure ? #49

Open
Nayte91 opened this issue Aug 30, 2022 · 12 comments
Open

use of dialog tag for modal structure ? #49

Nayte91 opened this issue Aug 30, 2022 · 12 comments

Comments

@Nayte91
Copy link
Contributor

Nayte91 commented Aug 30, 2022

Hello,

To begin with, thank you very much for your work on this hook. I'm going mad with react and the absurd difficulty of doing a simple HTML modal, I tried to do my component, and spent a lot of time on tutorials and repositories; For now, I may say that yours is by far the most satisfying out there.. Or at least the one my own style is the most aligned with :)

As my journey begun with the new tag - its new behavior - I was wondering if you already considered your Modal component around this tag ?

If not, let me give you some materials:

  • The dialog element comes with an interesting behavior, as it's hidden by default,
  • You can "open" it by calling 2 dedicated js functions, show() for a modalless behavior (you can click out, etc), or showmodal() for a strict user catch. I will continue around the second.
  • When one of those is used, an "open" attribute appears in the dialog element, and some properties change like making it visible, centered, on top of the Z pile. Also it's easy to style with like #modal-structure[open] { display: grid }
  • When showModal() is used, a new backdrop pseudo-element appears and makes it easy to style the background,

On a structure like yours, the dialog tag use can saves you both <div style={wrapperStyle}> and <div style={overlayStyle} />, quite interesting.

@Nayte91
Copy link
Contributor Author

Nayte91 commented Aug 30, 2022

The tricky part with React is that is not a thing to let a DOM node (a component) to live here, being hidden. But we need to fire the JS's showModal() function to enjoy the properties.

I found a little trick (I don't know how many rules it violates but..) to allow to display the modal with something like a boolean useState variable (isOpen) and fire the showModal() function: with 2 async functions and an awaited operation into.

const ModalStructure = forwardRef((props, ref) => {
    const [isOpen, setOpen] = useState(false)
    const modalRef = useRef()

    useImperativeHandle(ref, () => ({
        showModal: async () => {
            await setOpen(true)
            modalRef.current.showModal()
        },
        closeModal: async () => {
            await modalRef.current.close()
            setOpen(false)
        },
    }))

    return isOpen ? (
        <dialog
            id="modal"
            ref={modalRef}
            onClose={() => ref.current.closeModal()}
        >
            <header className="modal__header">
                <h1>Hello</h1>
                <button
                    className="close-btn"
                    type="button"
                    onClick={() => ref.current.closeModal()}
                />
            </header>
            <main>{props.children}</main>
        </dialog>
    ) : null
})```

@Nayte91
Copy link
Contributor Author

Nayte91 commented Aug 30, 2022

I think you have here all the valuable parts of my researchs, as the others (custom hook, portal, ...) are way better on your side.

If you want to give it a try, I'ld be happy!

@Nayte91
Copy link
Contributor Author

Nayte91 commented Sep 28, 2022

Hello @dc7290! I hope you are fine,

Following our discussion around my POC PR, I was concerned about your question: "For now, I have a feeling that the inconvenience of applying styles when using the dialog element may be a disadvantage.",

An elegant answer can be found in the :modal CSS pseudo class that makes styling convenient from outside of the component.

It actually has some support, that you can take in consideration.

Have a nice day,

@Nayte91
Copy link
Contributor Author

Nayte91 commented Dec 8, 2022

I put here the discussion and ideas instead of in a pull request:

Hello @dc7290 ,

Wonderfull! I will be the first user and fan of this new version!

  1. Unfortunately you are way stronger than me on React, so I can't really help technically,
  2. I think my PR with Proof of Concept has the core idea in it, especially those lines:
await setOpen(true);
document.getElementById('dialog-poc').showModal();

Beyond that, I can only give advices/wishes about the global API, what can be enjoyable to use.

Few ideas

Hook's API

As there is non-modal dialog elements and modal ones, I think we need to provide 2 custom hooks, where names must be choose :

  • useModal() // useNonModal() ?
  • useModal() // useDialog() ?
  • useModalDialog() // useNonModalDialog() ?
  • or having a parameter as an object with position for non modal dialog; if missing, then it's a modal (centered on screen)

I feel like this kind of pattern is a bit annoying and can be optimized:

const Test = () => {
    const {Dialog, openDialog} = useDialog()

    return (
        <>
            <div>
                <h1>My actual component</h1>
                <p>Lorem ipsum</p>
                <button onClick={() => openDialog()}>Click for modal</button>
            </div>

            <Dialog>
                <MyModalContent />
            </Dialog>
        </>
    )
}

I wanted to do such a thing:

const Test = () => {
    const {openDialog} = useDialog(<MyModalContent />)

    return (
        <div>
            <h1>My actual component</h1>
            <p>Lorem ipsum</p>
            <button onClick={() => openDialog()}>Click for modal</button>
        </div>
    )
}

But it seems that the 'MyModalContent' must output somewhere it can be rendered, and I can't in a hook. Maybe with a global DialogContext?

Styling the modal

For the style, I think the easiest way to apply is:

  1. to allow user to inject style in hook,
  2. document clearly what can be styled and how,

example of style injection:

import styles from './test.module.css'
import Modalstyles from './modal.module.css'

const Test = () => {
    const {openDialog} = useDialog(<MyModalContent />, Modalstyles)

    return <h1 className={styles.title}>Hello</h1>
}

But it works because I use CSS modules, with nextJS. Maybe this something to try?

@Nayte91
Copy link
Contributor Author

Nayte91 commented Dec 8, 2022

About the usability of tag on browsers

I did some tests with my browserstack (stayed on Laptop MacOS):

  • On Safari under 15.4, it doesn't work well. Safari is now 15.6, so I confirm your ~1% global
  • On Firefox, works very well since at least v100.0, and the backdrop works well, I don't know why this table says it's not, maybe a bug? I opened a ticket but I closed it since, because they corrected their table about ::backdrop, it works very well so it was a mistake on their side!
  • On Chrome & Edge, everything is fine since at least v100.0, and when I try to tab in a modal, the focus stays trapped, I switch between my only 3 buttons and my address bar,
  • On Opera, everything is fine since at least v85.0,

As you said the main concern is the cutting-edge Safari version for users.

As the focus trap, isolation, centering, keyboard are managed, I feel like <dialog> is a far better starting point than <div>. Even if it needs some further improvements and brings few limitations (i.e. default backdrop zone can't be clicked to close a modal), build over <dialog> seems to simplify a lot the hook.

Here's a good article about how to build around dialog.

@dc7290
Copy link
Member

dc7290 commented Dec 10, 2022

@Nayte91
Hi!

At least I made something that works, so I'm sharing!

I'm able to solve a lot of problems I was having using the DIALOG element.

  • Have to provide a focus-trap.
     - Bugs in re-rendering due to it
  • Decrease in the number of ModalWrapper dependencies returned by useModal
  • z-index considerations

@Nayte91
Copy link
Contributor Author

Nayte91 commented Jan 9, 2023

Hello @dc7290,

It works well! I like how it seems simple to use. Also the style's injection seems sweet.

I wonder if bodyscrolllock still relevant, as if you choose between ModalDialog (JS showModal()) or NonModalDialog (JS show()) it will define this behavior?

Anyway It's a great draft, and I'm impressed how fast you made it!

@dc7290
Copy link
Member

dc7290 commented Jan 18, 2023

@Nayte91

Thanks for looking!

I wonder if bodyscrolllock still relevant, as if you choose between ModalDialog (JS showModal()) or NonModalDialog (JS show()) it will define this behavior?

Indeed!!!
This is not necessary lol.

@dc7290
Copy link
Member

dc7290 commented Jan 25, 2023

@Nayte91
It has been available since v3.3.0!
https://github.com/microcmsio/react-hooks-use-modal/releases/tag/v3.3.0

@Nayte91
Copy link
Contributor Author

Nayte91 commented May 2, 2023

Hello!
I hope you are fine. And of course, many thanks for the feature. I'm using it on my NextJS project. Do you use it?

Just wanted to share with you this video from a famous frontend Youtuber who speak about . I'm happy that we are some months before him :)

Fortunately there is some nice tricks, especially about how to add a parameter to close the modal dialog when clicking outside. Can be a neat implementation!

@dc7290
Copy link
Member

dc7290 commented May 10, 2023

@Nayte91
It has been a long time!
How are you?

I see!
Nayte91 had the foresight to see this:)

I am curious about its implementation.
I am waiting for Nayte91's contribution lol.

@Nayte91
Copy link
Contributor Author

Nayte91 commented May 12, 2023

Hello! I'm really fine, I hope you are too!

I actively use the lib and the new modal on my NextJS project :)
Unfortunately I'm more a backend dev (PHP/Symfony) and I'm struggling with JS & React; I can dev a bit with, but contributing is harder.

Noneless I will try this one in june ;) I have the target code, I may succeed in it. I'll keep you in touch ! Waiting for Street Fighter 6 release, launching an app for this occasion, then I will have some time to dive into this.

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

Successfully merging a pull request may close this issue.

2 participants