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

proof of concept dialog element #61

Closed
wants to merge 1 commit into from
Closed

proof of concept dialog element #61

wants to merge 1 commit into from

Conversation

Nayte91
Copy link
Contributor

@Nayte91 Nayte91 commented Sep 7, 2022

Here's a proof of concept : this is the bare minimum changes in order to use dialog HTML element instead of div.

  • Notice the hard-coded id to get the dialog element and use showModal() function: I should use a forwardRef hook in order to have the open() function managed by the parent element. As it is too complicated to use for me (and maybe I'm missing something simplier), I'ld rather show you something dirty but that works :)
  • Notice the await before setOpen(true), that allows the async function to wait for React to render the modal before running the "showModal()" following line.

Also I'm sorry but I don't know how to open a pull request on a non-existing branch on your side; so please feel free to revoke this PR and try on my side.

closes #49

@dc7290
Copy link
Member

dc7290 commented Sep 8, 2022

@Nayte91

Thank you for creating this PR!

Using the dialog element was an idea I didn't have, so that was refreshing!
For now, I have a feeling that the inconvenience of applying styles when using the dialog element may be a disadvantage.
Can you tell us the advantages of using the dialog element?

@Nayte91
Copy link
Contributor Author

Nayte91 commented Sep 8, 2022

Hello @dc7290,

All about how dialog element works can be found in issue #49

I also created for you a test page to see how the dialog can be used: I would be honored if you take a look.

If you look at the source code, you will see how dead simple everything is:

  • The equivalent of your focusTrapOptions: true is done with showModal(), and focusTrapOptions: false with show() so I think you can get rid of your useFocusTrap() hook,
  • You can save the Overlay part, the role, aria-modal, tabIndex and style attributes of your wrapper,

I feel like the styling is also simplier, as the user can focus only on the modal content,

I feel the harder part is to open or close the modal from the parent component, but maybe it's easier with a hook ? As you are better than me, you will find a clean solution,

What do you think about my demo ? Have you some concerns with <dialog> again ?

@dc7290
Copy link
Member

dc7290 commented Sep 9, 2022

@Nayte91

Thank you for even creating a demo!

I think the idea of using the dialog element is great!
It's great that it doesn't require complicated implementation, especially with keyboard operations.

One thing I was wondering about is the browser behavior regarding supported browsers and focus traps.

Supported Browsers

  • Not functional on iOS Safari 15.3 or earlier (3.3% of Global)
  • Does not work in Safari 15.3 or earlier (1.02% globally)
  • ::backdrop does not work in Firefox

Focus Trap

  • In Chrome, pressing the TAB key on the last element in a modal causes focus to go outside the modal

However, I thought this suggestion was well worth incorporating.
If it is best implemented in terms of a11y with a dialog element, I'd like to incorporate it!

@Nayte91
Copy link
Contributor Author

Nayte91 commented Sep 9, 2022

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,
  • 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. I'll look further for Firefox backdrop.

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.

@Nayte91
Copy link
Contributor Author

Nayte91 commented Nov 8, 2022

Hello,

My ticket stills opened, but they corrected their table about ::backdrop, it works very well so it was a mistake on their side!

@dc7290
Copy link
Member

dc7290 commented Dec 6, 2022

@Nayte91
Hello!!

Sorry for the lack of time.
I am actually planning to rework the library itself using this idea.
So I wonder if you could give me some advice when that PR is created.

@Nayte91
Copy link
Contributor Author

Nayte91 commented Dec 6, 2022

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:
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?

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>Hello</h1>
}

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

@dc7290
Copy link
Member

dc7290 commented Dec 7, 2022

Thank you!!!

I'll share more when I'm able to prototype!

@dc7290
Copy link
Member

dc7290 commented Dec 7, 2022

Probably at the end of December.

@dc7290 dc7290 closed this Jan 25, 2023
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 this pull request may close these issues.

use of dialog tag for modal structure ?
2 participants