-
Notifications
You must be signed in to change notification settings - Fork 194
Optional animations #165
base: master
Are you sure you want to change the base?
Optional animations #165
Conversation
I was thinking about something like this, but left that out, because I couldn't see a need to dynamically dis/enable animations. I suppose an application will either animate or not. To disable animation, just declare empty animate-{row,column} classes. But I guess there could be a use case for that. |
I suppose it might be worth trying to turn the animation off on drag start and then turn it back on... |
Awesome, this looks great! Thanks for the tests. I do think that it'd be better to have animations on by default instead of off - that way you can still disable and it's not a major change. |
It occurred to me, that the purpose of the animations is really to animate the collapsing/toggling. So I would suggest that we add the classes before toggling a container and remove it afterwards. What do you think. |
Hm, that'd be interesting to see, mind putting together that in a Plunkr? |
@SomeKittens I'd like to do that later this week, if time permits. But will wait for @widmoser 's reaction since s/he seems to be very active at the moment. |
@petrsimon, @SomeKittens I turned on animations by default in the PR. I like the idea to limit animations to collapsing/toggling as it might also fix the issues reported in #161, but I think it would be better to create a separate PR for this. I can look into it at some point, but at the moment #166 has more priority for me. |
Ok, I will hold back :)
|
Something I was also thinking about is to have a look at whether it would be possible to simplify the collapse/toggle implementation and after that do everything related to it (persisting state, etc.), because right now I am not very confident in changing something in relation to it because I don't understand it fully. I'll probably have to examine the code in more detail :) |
@SomeKittens is there something else that needs to be done before merging this PR? |
@widmoser Hi, sorry. Work got busy and then it got worse. Trying to catch up now. |
Things look good, squash the commits into one and I'll bring it in. |
57556c9
to
3469c35
Compare
3469c35
to
845428c
Compare
This PR has been pending for so long. Dear Dev team, could you please consider review the change and merge into upstream? |
We need this, too. +1 vote for merging this! |
This pull request adds an attribute
animate
that needs to be set to"true"
in order for the animations to be activated.This can be used as a work-around for issue #161.