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

💄 Improve all dialog animations #4103

Merged
merged 8 commits into from May 4, 2021

Conversation

cp2004
Copy link
Member

@cp2004 cp2004 commented Apr 19, 2021

What does this PR do and why is it necessary?

Implements #4085, replaces the default slide-in-fade animation with a simple, more performant CSS animation.

How was it tested? How can it be tested by the reviewer?

Applying the patch, opening all the dialogs to view the animation. Make sure none of them are using the slide animation....

Any background context you want to provide?

For ages, I was annoyed by the speed of the animation, looked into it to try and find the slow thing, which was jQuery & BS2's JavaScript animations.

What are the relevant tickets if any?

Closes #4085

Screenshots (if appropriate)

See #4085 for details

TODO

  • Apply to all core dialogs. Probably only if the point below is addressed.
  • Figure out the closing of the dialogs
    • This is more difficult to do with CSS animations, but I'm working on it. If it is too difficult, we'll need to work out if it is worth it or whether instant-close is acceptable.

@github-actions github-actions bot added the approved Issue has been approved by the bot or manually for further processing label Apr 19, 2021
@cp2004
Copy link
Member Author

cp2004 commented Apr 26, 2021

@foosel what do you think about changing all the dialogs?

At the moment I am struggling to get a fade-out animation to work without diving deep into changing the Bootstrap 2 sources. So at the moment, there is a fade in and instant out. Can't hook into the close settings function to trigger an animation in the settings view model since it only covers clicking the settings button, but not the close X, outside the dialog or esc key either.

I definitely think this should be done for settings, since it is the heaviest dialog and is way too slow. But the rest are not as slow because they have relatively less content. Maybe I will experiment so we can see what it feels like.

@foosel foosel added this to To review in PR triage Apr 28, 2021
PR triage automation moved this from To review to To merge Apr 28, 2021
@foosel
Copy link
Member

foosel commented Apr 28, 2021

Frankly, I have no qualms with applying it to all core modals. It's faster, works, and the fading backdrop ain't that important really.

That being said, wasn't there some event triggered on the closing of a modal regardless of the way it is closed? Could this maybe help?

@foosel foosel moved this from To merge to WIP/Draft in PR triage Apr 28, 2021
@jneilliii
Copy link
Contributor

That being said, wasn't there some event triggered on the closing of a modal regardless of the way it is closed? Could this maybe help?

I think I tried using this before in a plugin and don't think it exposed correctly the callback.

@cp2004
Copy link
Member Author

cp2004 commented Apr 28, 2021

There is something like this:

$('#modal').on("hidden", () => {
   // Do something
})

But I think when I tried it, it fired ,after Bootstrap closed the modal. By which point, it is too late to start an animation. At least I think so. Will keep looking, and try and change most of the core dialogs.

I tried to find them all.... Hopefully got them.
Feels a bit snappier, but still enough animation.

Easily configurable if it needs adjusting.
@cp2004
Copy link
Member Author

cp2004 commented Apr 28, 2021

Having changes all the core dialogs to fade in/no animation out, it works. It feels just fine. And quicker & snappier still.

Ready for merge/review 🙂

Oh, and I reduced the animation duration slightly. It is now 300ms, down from 500. Maybe there is an optimum value, it's easily changeable.

@cp2004 cp2004 marked this pull request as ready for review April 28, 2021 21:12
@cp2004 cp2004 changed the title 💄 Improve settings dialog animations 💄 Improve all dialog animations Apr 28, 2021
@cp2004
Copy link
Member Author

cp2004 commented May 4, 2021

Only outstanding issue seems to be the webcam & timelapse loading - because the content gets loaded later, then the positioning is off. This is actually an issue with the current implementation if things go fast enough - the timing means I can get it to appear below.

I'll add some small CSS tweaks to hopefully predict the sizing of the webcam image and make it the correct size.

image

An interesting condition of when the correct positioning was calculated - with the animation, often the webcam stream loaded fast enough that it was before the animation was finished. Now there is no animation (from jQuery) then we hit a timing problem.

Yes, it is hardcoded but so is the modal width.
@foosel
Copy link
Member

foosel commented May 4, 2021

So do you consider this ready for merge or should I wait some more?

@cp2004
Copy link
Member Author

cp2004 commented May 4, 2021

It can be merged, it was luck that I found the final issue, I thought I was finished...

@foosel foosel merged commit 0580b0f into OctoPrint:maintenance May 4, 2021
PR triage automation moved this from WIP/Draft to Done May 4, 2021
@foosel foosel added this to the 1.7.0 milestone May 4, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Issue has been approved by the bot or manually for further processing
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants