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

Allow TransitionChild to passprops. Useful for component wrapper that… #4

Merged
merged 2 commits into from Jan 20, 2017

Conversation

khankuan
Copy link
Contributor

… clone children and modify props, new props should be passed down to child.

Reference: facebook/react#8522

… clone children and modify props, new props should be passed down to child.
@@ -157,7 +157,9 @@ class CSSTransitionGroupChild extends React.Component {
}

render() {
return React.Children.only(this.props.children);
const props = { ...this.props };
Object.keys(propTypes).forEach(key => delete props[key]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this pattern feels a bit odd to see here – i'm afraid of people using this with the babel optimization to strip prop types and hitting breakage

@jquense
Copy link
Collaborator

jquense commented Dec 10, 2016

hi there, thanks for the PR. I'm not sure I understand why this is needed. The user can already add any props to the children when they create them the first time. For a wrapper that controls/adjusts children why not wrap the TransitionGroup itself instead of the Component?

let me know if I'm misunderstanding

@khankuan
Copy link
Contributor Author

khankuan commented Dec 10, 2016

Here's the example that was unexpected:

<TransitionGroup component={List}>
   <ListItem>x</ListItem>
</TransitionGroup>

ListItem no longer has props that List added. Updated to remove specific props instead of using propTypes.

@khankuan
Copy link
Contributor Author

Hi, is this repo still maintained?

@jquense
Copy link
Collaborator

jquense commented Jan 11, 2017

is it maintained? yes, yes it is.

@khankuan
Copy link
Contributor Author

Is there anything blocking to have the PR merged? Thanks!

@jquense
Copy link
Collaborator

jquense commented Jan 11, 2017

I'm not sure this is the right approach... expectung the animation container to behave correctly when passing in arbitrary components (like List) seems fraught. who is to say that component will play nice. what if it doesn't render the children at all? in general it seems better for List to render a Transition group (ideally).

@taion thoughts here?

@jquense
Copy link
Collaborator

jquense commented Jan 11, 2017

(sorry for the delay btw)

@khankuan
Copy link
Contributor Author

I think the original intention of the component prop is to allow custom wrappers instead of the default div. If List is to support component prop, it would look like:

<List component={TransitionGroup}>
   <ListItem>x</ListItem>
</List>

The same functionality would need to be built into other wrappers, like a Grid.

I feel that its better practice for components that are most reusable to play nice rather than have dependencies above playing nice downwards.

TransitionGroup assumed to be the wrapper hence it enforce either a div or a custom component, so it make sense that the tradeoffs of the assumption is taken care of.

Alternatively, i propose TransitionGroup is be more like a decorator/enhancer kind of component that wraps a parent container rather than making its own div.

@taion
Copy link
Member

taion commented Jan 11, 2017

@jquense Taking React-Bootstrap as an example, how would we make this work with something like <Nav> and <NavItem>?

@jquense
Copy link
Collaborator

jquense commented Jan 11, 2017

yeah I guess it doesn't work in the case which isn't ideal. My concern here is that it may be misleading that it would work with any Component, which isn't true if the rendered root decides to do anything odd with the children. I guess its fine to let the buyer beware tho on those things.

@khankuan
Copy link
Contributor Author

I think the goal is to reduce the cases where adding transitions via TransitionGroup fail to work well. I don't think any component is meant to work with everything, imagine <TransitionGroup component={ReduxProvider}>. We probably want to support as much sensible cases as possible.

@jquense
Copy link
Collaborator

jquense commented Jan 20, 2017

fair enough

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

3 participants