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 controllable.js mixin #2889

Merged
merged 2 commits into from
Jan 12, 2016

Conversation

newoga
Copy link
Contributor

@newoga newoga commented Jan 12, 2016

This PR removes mixins/controllable and replaces functionality with local methods.

The mixins/controllable.js mixin adds a getValueLink() method to components and is currently used in two places:

  • Menu
  • Tabs

This commit moves the function locally into each of the components so that they don't depend on the controllable.js mixin. The functions should be removed when we're ready to officially deprecate valueLinks.

…ethods

The `mixins/controllable.js` mixin adds a `getValueLink()` method to components and is currently used in two places:

- Menu
- Tabs

This commit moves the function locally into each of the components so that they don't depend on the `controllable.js` mixin. The functions should be removed when we're ready to officially deprecate valueLinks.
@newoga
Copy link
Contributor Author

newoga commented Jan 12, 2016

Related to #2880

@oliviertassinari If we're ready to start the "deprecating valueLink" effort now, let me know and I can see if I can refactor the Menu and Tabs components in this PR to not use them.

@newoga newoga added the Core label Jan 12, 2016
@newoga
Copy link
Contributor Author

newoga commented Jan 12, 2016

@alitaheri Feel free to review this at your convenience too. Just trying to remove the "low hanging fruit" mixins 😄

@alitaheri
Copy link
Member

@newoga Thanks 😁 Are these imperative methods even used anywhere? They aren't documented anyway. think we can remove them entirely?

getValueLink I mean. They seems to be only used inside the component 😁

@newoga
Copy link
Contributor Author

newoga commented Jan 12, 2016

@alitaheri Yeah I agree, I didn't see them used anywhere except inside the components. I moved the method over as is just so it would work exactly as it did before. The method is called quite a few times internally. I think the best time to remove would be when we start conceptually deprecating the valueLink for each component as mentioned in #2880. If we're ready to start that now, I can modify this PR and try to get this method out 😄

@alitaheri
Copy link
Member

@newoga No need. But I think it wouldn't hurt to add a comment explicitly stating that we will remove them so no one would rely on them until then

// Remove with valueLink

?

@newoga
Copy link
Contributor Author

newoga commented Jan 12, 2016

@alitaheri Good idea, I'll add a comment for now 👍

@alitaheri
Copy link
Member

@newoga Thank you ( ^_^ )

@oliviertassinari
Copy link
Member

This PR removes mixins/controllable and replaces functionality with local methods.

That's a very good step.

If we're ready to start the "deprecating valueLink" effort now

Well, I think that we can start working on it 👍.

oliviertassinari added a commit that referenced this pull request Jan 12, 2016
@oliviertassinari oliviertassinari merged commit f7162c1 into mui:master Jan 12, 2016
@rob0rt rob0rt mentioned this pull request Jan 12, 2016
7 tasks
@newoga newoga deleted the controllable-mixin-removal branch January 12, 2016 20:05
@newoga newoga restored the controllable-mixin-removal branch January 26, 2016 13:21
@newoga newoga deleted the controllable-mixin-removal branch January 26, 2016 14:07
@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.

4 participants