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

[TimePicker] Remove style-propable mixin #3188

Merged
merged 1 commit into from
Feb 5, 2016

Conversation

newoga
Copy link
Contributor

@newoga newoga commented Feb 5, 2016

No description provided.

inner: this.isInner(nextProps.value),
muiTheme: newMuiTheme,
inner: isInner(nextProps),
muiTheme: nextContext.muiTheme ? nextContext.muiTheme : this.state.muiTheme,
Copy link
Member

Choose a reason for hiding this comment

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

nextContext.muiTheme || this.state.muiTheme?
But that's a minor point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, I like that more. I'll switch to that since I'm going through.

@oliviertassinari
Copy link
Member

@newoga That loos good 👍.

@newoga
Copy link
Contributor Author

newoga commented Feb 5, 2016

Mm, not sure why this build failed. Timing issue maybe, I ran the tests locally again newoga@0e2b22f and can't reproduce the fail.

@mbrookes
Copy link
Member

mbrookes commented Feb 5, 2016

I've seen that on another PR. Make a trivial change and push to force a re-run. Or we just ignore the error.

@newoga
Copy link
Contributor Author

newoga commented Feb 5, 2016

I've seen that on another PR. Make a trivial change and push to force a re-run. Or we just ignore the error.

Interesting, okay thanks @mbrookes!

@newoga
Copy link
Contributor Author

newoga commented Feb 5, 2016

Done, switch the setState to use nextContext.muiTheme || this.state.muiTheme.

@alitaheri
Copy link
Member

I've seen that on another PR. Make a trivial change and push to force a re-run. Or we just ignore the error.

Also, those with push access to the repository can login to the travis-ci (with their github account) and manually restart the build. It's cleaner and faster.

@@ -64,7 +64,7 @@ const ClockNumber = React.createClass({
pos = pos / 5;
}

let positions = [
const positions = [
Copy link
Member

Choose a reason for hiding this comment

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

any reason why these are instantiating with each render? I don't see any.

Copy link
Member

Choose a reason for hiding this comment

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

Wow, indeed 😨.

@alitaheri
Copy link
Member

This is some hard work 👍 👍 Although I'm ok with merging this as it is. we'll probably change the way we calculate the styles soon after we figure out how we should implement memoization.

@oliviertassinari
Copy link
Member

I agree, let's do one thing at the time.
@newoga Thanks for your hard work!

oliviertassinari added a commit that referenced this pull request Feb 5, 2016
[TimePicker] Remove style-propable mixin
@oliviertassinari oliviertassinari merged commit 246dbeb into mui:master Feb 5, 2016
@alitaheri
Copy link
Member

🎉 🎈 🎉 awesome work @newoga 👍

@newoga
Copy link
Contributor Author

newoga commented Feb 5, 2016

Thanks guys! 🎉
Yeah as I've been going through many of the components, I'm see a lot of areas that could use improvement (esp as it relates to performance). Though I will try to stay focused and keep changes related to migration of mixin to context.

@newoga newoga deleted the #2852/time-picker branch February 6, 2016 23:24
@zannager zannager added component: time picker This is the name of the generic UI component, not the React module! component: pickers This is the name of the generic UI component, not the React module! labels Mar 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: pickers This is the name of the generic UI component, not the React module! component: time picker This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants