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

feat(Transition): Add component #813

Closed
wants to merge 12 commits into from
Closed

feat(Transition): Add component #813

wants to merge 12 commits into from

Conversation

layershifter
Copy link
Member

@layershifter layershifter commented Nov 9, 2016

WIP

Fixes #200.

TODO

  • Work with children including interval and reverse;
  • looping and animation on load;
  • callbacks for animation events;
  • add direction for overring active;
  • skip in/out from active on static animations.

@codecov-io
Copy link

codecov-io commented Nov 9, 2016

Current coverage is 98.08% (diff: 6.52%)

Merging #813 into master will decrease coverage by 1.91%

@@           master       #813   diff @@
========================================
  Files         137        137          
  Lines        2244       2243     -1   
  Methods         0          0          
  Messages        0          0          
  Branches        0          0          
========================================
- Hits         2244       2200    -44   
- Misses          0         43    +43   
  Partials        0          0          

Powered by Codecov. Last update 194e02d...5651e2b

@layershifter
Copy link
Member Author

layershifter commented Nov 9, 2016

@levithomason I've added initial example, looks that it feels good with our augmentaion pattern 👍 Main problem that we need have there animation control like in jQuery:

$('.autumn.leaf').transition('scale');

I'm thinking about binding internal method of Transition:

render() {
  let control
  const element = <Transition as='div' contol={control}>Test</Transition>

  return (
    <div>
     {transtion}
     <Button onClick={control}>Run</Button>
   </div>
  )
}

But it looks horrible 😢 We can clone there pattern with trigger, but it will be perfect to get elegant way for animation controlling.

@levithomason
Copy link
Member

But it looks horrible 😢 We can clone there pattern with trigger, but it will be perfect to get elegant way for animation controlling.

I'm not sure I follow this, could you expound?

Animating Multiple Components

I think we should use children here. It seems there are several usage examples showing animations for multiple components at once: http://semantic-ui.com/modules/transition#/examples. Some are very useful, such as animating sequences of elements.

In/Out

I think we'll need to support in/out animations right away as well. There will likely be tricky things to deal with here regarding out animations and component unmount.

@levithomason
Copy link
Member

I added all the animations and an "Explorer" example for easier testing. We can use this as we build out the component.

I also think the active and visible props should be removed and those classNames should be managed by the component. I think classes should be added/removed on mount/unmount and after the duration has expired.

@layershifter
Copy link
Member Author

@jcarbo @levithomason I've added more animations and reworked component, take a look and punish me 👍

Animating Multiple Components

Yes, I agree with you there, it's next step.

@levithomason
Copy link
Member

levithomason commented Nov 13, 2016

This is looking really good so far 👍

I spent some time looking into ReactCSSTransitionGroup and others. It appears the Facebook team will no longer support ReactCSSTransitionGroup, however, the react-bootstrap team has offered to continue support. They will be transferring their Transition component to the reactjs org. It seems to do most or all of what we're about to do here. We may be able to drop it in and wrap it, for now, then later if our needs outgrow it we can maintain our own (much like we did with our Portal). It is probably worth taking a look at that Transition component, even if only for inspiration.

Triggering animations

Currently, animations are triggered by toggling active. I think we should also animate on mount / unmount by default. So, the Transition component would default active to false, and on did mount it would trySetState active: true. This also means becoming an AutoControlledComponent. There are issues with "out" animations in the community as whole since React does not provide a way to defer the unmount so not sure how we go about this yet. Perhaps, we tell users to first animate out, then unmount their components (which leads me to my next point)

Callbacks

I think we need callbacks for every stage of the animations, reference react-bootstrap Transition props.

Potential queue bugs

I notice when rapidly clicking the run button, sometimes the animations get out of sync. I'm not sure what the issue is there yet.

Will wait for the children update to further review, keep it up!

@layershifter
Copy link
Member Author

I spent some time looking into ReactCSSTransitionGroup and others.

Main problem that I see with ReactCSSTransitionGroup is animation triggering. In current implemention we can do this with switch of active prop, while ReactCSSTransitionGroup performs onEnter/onLeave. Did I overlooked something?

I notice when rapidly clicking the run button, sometimes the animations get out of sync.

Yep, it look like issue with hot reload.

@levithomason
Copy link
Member

Yep, it look like issue with hot reload.

Hm, there shouldn't be any reloading involved, I'm only clicking "Run" in the docs but not changing any code. There is no rebuild happening. Let me know if I'm missing the point here.

Alexander Fedyashov added 2 commits November 15, 2016 12:08
…antic-UI-React into feat/transition

Conflicts:
	docs/app/Examples/modules/Transition/Transitions/TransitionExampleScale.js
	docs/app/Examples/modules/Transition/Transitions/index.js
	src/modules/Transition/Transition.js
@layershifter
Copy link
Member Author

layershifter commented Nov 15, 2016

animations get out of sync

Fixed, problem was with array.pop in queue, while there must be array.shift 😄

Callbacks

I think we prefer SUI callbacks, so I added them.

@levithomason
Copy link
Member

Awesome, will give it a whirl as soon as I can :)

@levithomason
Copy link
Member

Looking good so far 👍 Will wait for docs on the callbacks / direction to review those. There are a couple things I'm not sure about yet:

  1. The active prop vs direction vs ?????. React bootstrap uses in={true|false}. IMHO, active is a little confusing since it seems a transition is "active" while it is in transition, at least to me. I kind of like in because in={true} makes it clear it has transitioned "into the view" and in={false} makes it clear that it has transitioned "out of the view".
  2. Regarding callback arguments, I'm leaning toward preserving the "second param is always a data object" pattern, RFC: all event handlers should callback with (event, data) #623. Wondering if we then callback with onFoo(null, { ...props, ...state }) or similar. The alternative would be onFoo({ ...props, ...state }), though I don't think we should pass these separately as it is now, onFoo(props, state).

@layershifter
Copy link
Member Author

Basically, I wanted something that makes visible > animation > hidden or hidden > animation > visible, current logic makes it perfectly with controlling active prop (yes, we can change it to in). But, we have there pain with static animations and manual direction:

$('.leaf').transition('jiggle')
$('.leaf').transition('horizontal flip in') // Specifying a direction example
<Transition active={active} animation='jiggle' />
<Transition active={active} animation='horizontal flip' direction='in' />
<Transition in={in} animation='jiggle' />
<Transition in={in} animation='horizontal flip' direction='in' />

In fact, we have there visible > animation > visible, so control via switch of active/in prop looks very curious 😧 Any ideas, what we can do there?

@levithomason
Copy link
Member

I've marked this for review so I can come back to it and give it the time it deserves.

@cdaringe
Copy link
Contributor

cdaringe commented Jan 4, 2017

This looks awesome!

@cdaringe
Copy link
Contributor

cdaringe commented Jan 4, 2017

looks like live edit in the docs crash the app when you remove children from the Transition:

transition-live-edit-biff mov

@cdaringe
Copy link
Contributor

cdaringe commented Jan 4, 2017

it also seems to be unfriendly to the case if you use plaintext, vs a DOMElement/Component as a child

@cdaringe
Copy link
Contributor

cdaringe commented Jan 4, 2017

my last interruption on your PR for the day--the SUI transition docs aren't loading for me ATM (on a plane). i'm observing that the slide Transition here retains its node size. i expected on slide-out for the width or height to go to 0. is an intended supported use case to actually dynamically shrink/remove the node?

@layershifter
Copy link
Member Author

I'll back to this issue soon, I'll have some ideas on it and want to try them.

@levithomason
Copy link
Member

@layershifter I'd like to get a basic version of this merged. What scope could we cut to get an MVP version out? The only concern I'd have is ensuring the MVP API doesn't box us in from being able to add the rest of the features we need.

@layershifter
Copy link
Member Author

@levithomason I'll try to find enough time on this weekend.

@levithomason
Copy link
Member

@layershifter did you mean to close this PR?

@layershifter
Copy link
Member Author

I described the reason for the closure there.

@levithomason
Copy link
Member

I did read the note, but, perhaps I'm not understanding correctly. I would think the bulk of the work in this PR would be reusable and that we'd only need to update it rather than closing it.

@layershifter
Copy link
Member Author

layershifter commented Jan 29, 2017

Technically, my initial ideas were defeated. So, I think that Transition PR must be made from scratch with ReactTransitionGroup. And I don't see there any code that be reused in new implementation 😕

I have the vision of this component, but I want finish other issues (#524, #1072) before. I closed it to allow someone make the right MVP of Transition component. However, I'll open new PR if no one will pick up it.

@layershifter layershifter deleted the feat/transition branch June 7, 2017 12:17
@padrefuture
Copy link

Wish these transitions worked on React/Semantic UI...I was excited to use Semantic until I found out the transitions (or any animation) were non existent.

@Semantic-Org Semantic-Org locked and limited conversation to collaborators Nov 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants