search

Nycto / PicoModal

A small, self-contained JavaScript modal library. Plain, vanilla JS.
MIT License
241 stars 42 forks source link

speed up pico #24

Open cmnstmntmn opened 7 years ago

cmnstmntmn commented 7 years ago

hey,

i noticed that the way pico shows/hides the modal is via display:none/block property. this method is very expensive since it involves DOM redraw.

i've read how-to-achieve-60-fps-animations-with-css3

and would be great to switch to transform and opacity way of doing things.

Nycto commented 7 years ago

I would support this change. Pull requests are certainly welcome.

I'll need to consider whether this would require a major version bump, though. I'm thinking "yes", since it would be a significant change in behavior.

This also makes me think that an example using css animations should probably be added to the README.

cmnstmntmn commented 7 years ago

i've made an example by reseting the default style that comes with pico

i believe that css should be shipped separate, maybe in examples, in order to keep pico as light as possible and easy to compose on it.

still, i found a bug regarding bodyOverflow; if you run the fiddle several times (open close, open the second modal) you'll notice that the body still has overflow:hidden even if all modals were closed.

jordanlev commented 7 years ago

I think "very expensive" is rather subjective -- I've certainly never had any performance concerns with picoModal in my projects. Consider the possibility that this might be a micro-optimization with more drawbacks than benefits... because the display:none method is more straightforward (easier to understand what exactly it's doing), less error-prone, and not as "hacky" (in other words, it's using the correct property for its intended purpose).

At the very least if you decide to go down this route, please be aware of accessibility concerns... simply using transform and/or opacity to show/hide the modal might not actually make the content "disappear" to screen readers -- you'll probably want to utilize the "aria-hidden" attribute as well.