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

[Core] Remove style-propable mixin from transition-groups #2909

Merged
merged 1 commit into from
Feb 4, 2016

Conversation

newoga
Copy link
Contributor

@newoga newoga commented Jan 12, 2016

I'm going to try my best to do this methodically.

Related to #2852.

Two of these components now no longer have mixins! Two of them use the PureRenderMixin. Do you think we should replace the PureRenderMixin with shallow-compare in preparation of moving these to classes?

@newoga newoga added the Core label Jan 12, 2016
@newoga newoga added PR: Needs Review PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels Jan 12, 2016
@newoga
Copy link
Contributor Author

newoga commented Jan 12, 2016

Don't merge this by the way. Think we should iron out some syntax details mentioned in #2907 before doing mass conversion away from style-propable.

@oliviertassinari
Copy link
Member

Once #3124 is merged, we can start again this PR.

@newoga
Copy link
Contributor Author

newoga commented Feb 3, 2016

Yup, I have a few branches that started this. Looking forward to picking this back up. I also have a local branch that takes all the components that are using style/utils directly and switching them to context and Object.assign. Hopefully then we can deprecate and/or remove style/utils.js

@newoga newoga added PR: Needs Review and removed PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Feb 4, 2016
@@ -91,8 +89,7 @@ const SlideIn = React.createClass({

return (
<ReactTransitionGroup
{...other}
Copy link
Member

Choose a reason for hiding this comment

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

Can we keep it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the catch, that was not intentional 👍

@oliviertassinari
Copy link
Member

I'm happy with it, I would say let's squash down and merge 🎉.

This commit removes the `style-propable` mixin from `transition-groups` components. It replaces `this.mergeStyles()` with `Object.assign` and `this.prepareStyles()` with the same implementation that is stored in context. In addition, this commit replaces usage of `let` with `const` when the variable is not reassigned.

Signed-off-by: Neil Gabbadon <neil.gabbadon@emikra.com>
@newoga
Copy link
Contributor Author

newoga commented Feb 4, 2016

Squashed here too! If all is green, feel free to merge 😄

@oliviertassinari
Copy link
Member

@newoga Awesome, let's start the migration 🚜.

oliviertassinari added a commit that referenced this pull request Feb 4, 2016
[Core] Remove style-propable mixin from transition-groups
@oliviertassinari oliviertassinari merged commit fc57430 into mui:master Feb 4, 2016
@newoga
Copy link
Contributor Author

newoga commented Feb 4, 2016

@newoga Awesome, let's start the migration 🚜.

Yay, here we go! 🎉 🏃

@newoga newoga deleted the #2852-transition-groups branch February 6, 2016 15:30
@zannager zannager added the core Infrastructure work going on behind the scenes label Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants