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

Refactor API #24

Merged
merged 9 commits into from Jun 12, 2017
Merged

Refactor API #24

merged 9 commits into from Jun 12, 2017

Conversation

jquense
Copy link
Collaborator

@jquense jquense commented Apr 3, 2017

This is an initial stab at a new api for these components. The goals here are:

Goals:

  • Provide better low level hooks, while also avoiding the wonky childFactory pattern.
  • Write the TransitionGroup component in terms of a more primitive Transition component
  • Separate out the mount/transition state machine from implementations (like CSS)
  • Simplify the user facing API and providing something a bit more idiomatic React.

Changes

  • Adds a new component Transition that handles the animation of a single child.
  • Simplify TransitionGroup, deferring a lot of state management to Transition which are now required as children of TransitionGroup
  • Remove CSSTransitionGroup, replacing it's functionality with the new CSSTransition component, which can be used with TransitionGroup

These changes effectively move the transition definitions to the list items instead of list. It's a bit more verbose (easily encapsulated in a component, however), but it provides greater flexibility. For instance you can now easily mix and match transition types for list items, some could Fade, and others slide in. It also provides an opportunity to reuse and share transitions a bit easier. For instance, the react-bootstrap Fade component can be implemented in terms of a Transition, and used by itself to animate a single component, or with TransitionGroup to animate lists of things.

Example

<TransitionGroup>
  {listItems.map(item => (
    <CSSTransition 
       key={item.id} 
       timeout={500}
       classNames="fade"
     >
      <div>{item.name}</div>
    </CSSTransition>
  ))}
</TransitionGroup>

with the following css

  .fade-enter,
  .fade-exit {
    transition: opacity 500ms;
  }
  .fade-enter,
  .fade-exit-active {
    opacity: 0;
  }
  .fade-exit,
  .fade-enter-active {
    opacity: 1
  }

The removal of the various static method hooks also means it's much easier to encapsulate animations in Components. So we could write the above as

function Fade() {
  return (
    <CSSTransition
      {...this.props}
      classNames="fade"
      timeout: 500,
    />
  );
}

Implications and Concerns

This model complicates concept of appear transitions. Since it's the Transition are always being mounted in a TransitionGroup as items enter, they are always "appearing". Because the list items are controlling the transition, they don't know whether they are appearing or entering in relation to the parent list. This is somewhat solvable, but a bit ugly. I'd prefer to remove the concept of "appear" from the items in the context of a TransitionGroup, instead maybe just animation the TransitionGroup component itself, for appear animations

<FadeOnAppearTransition>
  <TransitionGroup>
     {items.map(item => (
       <SlideTransition key={item.id}><div>{item}<div></SlideTransition>
     ))}
  </TransitionGroup>
<FadeOnAppearTransition>

Not sure. I'd love some feedback from folks on when and if they use appear animations as separate from the enter animations

I still need to figure out how the API will play with the TransitionGroup component, i do still want the shortcut apis for defining timeouts and enabled/disabled animations in one spot up top.

cc @taion (I moved Transition from react-overlays btw)

@jquense
Copy link
Collaborator Author

jquense commented Apr 3, 2017

appear is weird to handle in this api...I don't love the awkwardness of it

@BowserKingKoopa
Copy link

BowserKingKoopa commented Apr 4, 2017

I'd prefer if transitions, especially transitioning out (since you can do transitioning in currently), was part of the Component life cycle so a component could handle its own transitions.

@jquense
Copy link
Collaborator Author

jquense commented Apr 4, 2017

@BowserKingKoopa ideally that would be great. Unfortunately there isn't any real way to do that, since unmounting is not asynchronous, and we cannot do it over time, which is part of why this s so complicated to do in React in the first place :)

The idea tho behind this Api is that you can get pretty close to that, by defining your own Transition component that is in charge of doing the actual transition, and it exposes a lot of hooks for each transition state change.

@todorone
Copy link

I guess it's a good direction. Reusable animations are a big plus. And separating appear from enter animations seems not necessary.

virgofx and others added 3 commits June 12, 2017 11:55
* Incorporated development + production builds into /dist (#64) and
updated Readme to include CDN/External information.

* Dropping references to Webpack specific build configurations in Readme
and adding clarity for CDN/external.
* Fixes #69: Adding capability to reduce production build output size and
allow users that import this package to include prop types in
development and optionally remove them using Webpacks UglifyJS + Define
plugin.

* Updated .babelrc that properly wraps prop-types.

* Restoring the original constant declaration for prop types as uglify
does find dead code and remove it.

* Updating changelog.
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

Successfully merging this pull request may close these issues.

None yet

4 participants