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.

Sign up for GitHub

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

Jump to bottom

[Core] Remove context-pure mixin #3331

Merged
merged 1 commit into from
Feb 15, 2016
Merged

[Core] Remove context-pure mixin #3331

merged 1 commit into from
Feb 15, 2016

Conversation

newoga
Copy link
Contributor

@newoga newoga commented Feb 14, 2016

Related to #2437
Resolves #2837

Can you guys take a look at this when you get a chance?

There's current a regression in this PR where when you hover over a <FlatButton />, the hovered color doesn't disappear when you hover off. It seems to be related to assigning undefined to backgroundColor key isn't clearing the color (it should probably be set to transparent when not hovered).

I just don't get how the current implementation is different than the one using context-pure because it was working before.

@alitaheri Found the bug! 🐛

Edit: Do we need to deprecate context-pure or can we just remove it? It's removed in this PR. We decided to remove it.

@newoga newoga added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI Core labels Feb 14, 2016
@alitaheri
Copy link
Member

No lets remove it. 👍

Also, i think it's used in the tests.

@newoga
Copy link
Contributor Author

newoga commented Feb 14, 2016

No lets remove it. 👍

Sounds good.

Also, i think it's used in the tests.

Thanks!

Yeah still not quite sure why flat-button isn't working yet...

alitaheri
alitaheri reviewed Feb 15, 2016
View reviewed changes
src/flat-button.jsx
} = this.constructor.getRelevantContextKeys(this.state.muiTheme);

button: {
color: buttonColor,
Copy link
Member

Choose a reason for hiding this comment

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

this is wrong. it should come from flatButton.color not button.color

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Geez, I looked at the diff 100 times and for some reason I could not see this 😡. Thanks! This fixed everything. 👍

Copy link
Member

Choose a reason for hiding this comment

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

glad I could help 😊

@newoga
Copy link
Contributor Author

newoga commented Feb 15, 2016

@alitaheri Thanks for finding that one difference. I tested it and it works, I'm going to resolve conflicts and squash...

@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 15, 2016
@newoga newoga changed the title [WIP] [Core] Remove context-pure mixin [Core] Remove context-pure mixin Feb 15, 2016
@yongxu yongxu mentioned this pull request Feb 15, 2016
Closed
8 tasks
@newoga
Copy link
Contributor Author

newoga commented Feb 15, 2016

Alright, this should be ready. 😄

I did a search for mixins: in src with this branch. 16 matches, (10 of which only have PureRenderMixin). We're really close. I think the only barriers to converting the codebase are: click-awayable, style-resizable, and isMounted() uses

@alitaheri
Copy link
Member

Yeah 👍 This is awesome 😎

Let's have @oliviertassinari Take a final look at it too. and merge 😍

oliviertassinari
oliviertassinari reviewed Feb 15, 2016
View reviewed changes
src/date-picker/date-picker-dialog.jsx
@@ -148,7 +129,7 @@ const DatePickerDialog = React.createClass({

const {
calendarTextColor,
} = this.constructor.getRelevantContextKeys(this.state.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.

this.state.muiTheme.datePicker?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah this is a bug. good catch 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you!

@oliviertassinari
Copy link
Member

@newoga Thanks for doing it! I'm the one how started adding the ContextPure mixin 5-6 months ago 😁. I'm glad we have found a better strategy 🎉.

@newoga
Copy link
Contributor Author

newoga commented Feb 15, 2016

  • Fixed theme variable assignment in date-picker-dialog

@oliviertassinari No problem, the evolution of this project has been a group effort! 🎉

@alitaheri
Copy link
Member

This is great 👍

alitaheri added a commit that referenced this pull request Feb 15, 2016
@alitaheri
Merge pull request #3331 from newoga/#2837/remove-context-pure
5a23f87 
[Core] Remove context-pure mixin
@alitaheri alitaheri merged commit 5a23f87 into mui:master Feb 15, 2016
@newoga newoga deleted the #2837/remove-context-pure branch February 15, 2016 17:51
@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
Reviewers
No reviews
Assignees
No one assigned
Labels
core Infrastructure work going on behind the scenes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Core] remove context-pure mixin from components
4 participants
@newoga @alitaheri @oliviertassinari @zannager