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

Modal component is relying too much on Javascript #458

Open
Atmos4 opened this issue Feb 17, 2024 · 9 comments
Open

Modal component is relying too much on Javascript #458

Atmos4 opened this issue Feb 17, 2024 · 9 comments

Comments

@Atmos4
Copy link

Atmos4 commented Feb 17, 2024

Currently there is a strong dependency on Javascript with the modal component, especially with animations and disabling scroll. Both can be done with pure CSS instead.

  1. Animations can be done with CSS transitions

  2. Disabling scroll can be achieved with CSS :has pseudo-class.

:has is relatively new in terms of browser support, but it seems to be used in the codebase already so it's probably not a concern.

Here is a link to the demo on CodePen.

Let me know what you think. If you like the idea, I can submit a PR.

@lucaslarroche
Copy link
Member

:has is relatively new in terms of browser support, but it seems to be used in the codebase already so it's probably not a concern.

Until then I had used :has as little as I could. But since ~December 2023, we can consider that we are free to use it wherever we need it. ~91.96% usage.

@lucaslarroche
Copy link
Member

Here is a link to the demo on CodePen.

Great demo

If you like the idea, I can submit a PR.

@Atmos4, I love it.

Let's also add button[formmethod="dialog"] to target the close button, but keep .close and [rel="prev"] to avoid breaking changes.

@Atmos4
Copy link
Author

Atmos4 commented Feb 18, 2024

Nice I will start working on that then!

Let's also add button[formmethod=dialog]

See my comment on the previous issue about the limits of button[formmethod] as close icon.

@Atmos4
Copy link
Author

Atmos4 commented Feb 28, 2024

There is one issue with this @lucaslarroche: if I want closing transitions to display properly, I need to always have display != none and visibility: visible, which will always make the content of the modal focusable.

This can be fixed with visibility: hidden and transition-behavior: allow-discrete, but browser support is very bad (i.e. doesn't work on Firefox)

In my opinion it's still an upgrade from the current CSS behavior, and transition-behavior will gracefully degrade to no animation so I guess it's OK? What do you think?

@pietz
Copy link

pietz commented Mar 27, 2024

I'm doing this and it works perfectly (except for click outside to close). Might be worth sharing it in the docs:

<dialog id="modal">
  <article>
    <header>
      <button aria-label="Close" rel="prev" onclick="modal.close()"></button>
    </header>
    <p>...</p>
  </article>
</dialog>

<button onclick="modal.showModal()">Open Modal</button>

@Atmos4
Copy link
Author

Atmos4 commented Mar 28, 2024

@pietz This is fine, but you will never have closing animations displaying properly... unless you always display the modal, which makes it always focusable (see my previous comment).

@pietz
Copy link

pietz commented Mar 28, 2024

@Atmos4 That's true and I don't really care if it's added to the docs or not. However, given Picos philosophy and design principles, I think that my provided solution is the closest to what the average PicoCSS user expects to see: Preferring very simple HTML-only solutions that might miss a feature or two over complex or cryptic feature complete solutions. I might be wrong though :)

@Atmos4
Copy link
Author

Atmos4 commented Mar 29, 2024

@pietz I think we pretty much agree on most things here :) see the demo link I posted in the issue, it looks like your solution regarding JS is roughly the same as mine.

I will publish a PR about the animations and changes to the modal. I can try to update the docs too :)

@Atmos4
Copy link
Author

Atmos4 commented Mar 29, 2024

FYI here is the related PR - #503

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

No branches or pull requests

3 participants