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

speed up pico #24

Open
cmnstmntmn opened this issue Jan 10, 2017 · 3 comments
Open

speed up pico #24

cmnstmntmn opened this issue Jan 10, 2017 · 3 comments

Comments

@cmnstmntmn
Copy link

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
Copy link
Owner

Nycto commented Jan 10, 2017

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
Copy link
Author

cmnstmntmn commented Jan 18, 2017

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
Copy link

jordanlev commented Feb 4, 2017

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.

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