Skip to content
This repository has been archived by the owner on Sep 8, 2020. It is now read-only.

Optional animations #165

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

widmoser
Copy link
Contributor

@widmoser widmoser commented Dec 4, 2015

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.

@petrsimon
Copy link
Contributor

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.

@petrsimon
Copy link
Contributor

I suppose it might be worth trying to turn the animation off on drag start and then turn it back on...

@SomeKittens
Copy link
Contributor

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.

@petrsimon
Copy link
Contributor

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.

@SomeKittens
Copy link
Contributor

Hm, that'd be interesting to see, mind putting together that in a Plunkr?

@petrsimon
Copy link
Contributor

@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.

@widmoser
Copy link
Contributor Author

widmoser commented Dec 9, 2015

@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.

@petrsimon
Copy link
Contributor

petrsimon commented Dec 9, 2015 via email

@widmoser
Copy link
Contributor Author

widmoser commented Dec 9, 2015

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 :)

@widmoser
Copy link
Contributor Author

widmoser commented Jan 5, 2016

@SomeKittens is there something else that needs to be done before merging this PR?

@SomeKittens
Copy link
Contributor

@widmoser Hi, sorry. Work got busy and then it got worse. Trying to catch up now.

@SomeKittens
Copy link
Contributor

Things look good, squash the commits into one and I'll bring it in.

@stanleyxu2005
Copy link

This PR has been pending for so long. Dear Dev team, could you please consider review the change and merge into upstream?

@jcford73
Copy link

We need this, too. +1 vote for merging this!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants