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 the deprecated API of v0.14.x #3108

Merged
merged 1 commit into from
Feb 4, 2016
Merged

[Core] Remove the deprecated API of v0.14.x #3108

merged 1 commit into from
Feb 4, 2016

Conversation

oliviertassinari
Copy link
Member

This PR is in preparation for the next 0.15.0 release.

Edit by @newoga:

0.14.0 Deprecations

This is a list of deprecations that were added in version 0.14.0. I recommend that we remove these in our first 0.15.0 release candidate.

Deprecated components
  • menu/menu (including related components)
  • lists/list-divider (Remove list-divider export from lists/index.js)
  • menus/menu-divider
  • drop-down-icon (PR #2994) (There is still logic checking for DropDownIcon in toolbar-group.jsx)
Deprecated imperative methods
  • Dialog: _getAction()
  • DropDownMenu: getInputNode() Needed by the TextField component.
  • LeftNav: toggle(), open(), close()
  • Snackbar: show(), dismiss()
  • TextField: setErrorText(), setValue()(Need to figure out what to do with clearValue())
Deprecated props
  • Dialog: actionFocus, width
  • DropDownMenu: displayMember, labelMember, menuItems, selectedIndex, valueLink, valueMember
  • SelectField: labelMember, menuItems, selectedIndex
  • IconMenu: closeOnItemTouchTap (not deprecated in docs)
  • LeftNav: header, menuItemClassName, menuItemClassNameLink, menuItemClassNameSubheader, menuItems, onChange, onNavClose, onNavOpen, selectedIndex
  • Snackbar: onDismiss, onShow, openOnMount

@oliviertassinari oliviertassinari changed the title [Core] Remove the deprecated API [Core] Remove the deprecated API (and as many dead code as I could find) Jan 30, 2016
@alitaheri
Copy link
Member

−1,919 !!! 😱 😱 😱

@alitaheri
Copy link
Member

Looks good so far. I'll have a deeper look tomorrow. We should be careful with this one 😅 😅

@oliviertassinari
Copy link
Member Author

@newoga Thanks for doing https://gist.github.com/newoga/1a604b642fd6a52e03a4. I have done a second commit to remove the missing deprecated API.

@oliviertassinari oliviertassinari added this to the 0.15.0 Release milestone Jan 31, 2016
@newoga
Copy link
Contributor

newoga commented Jan 31, 2016

@newoga Thanks for doing https://gist.github.com/newoga/1a604b642fd6a52e03a4. I have done a second commit to remove the missing deprecated API.

No problem, I'm going to check this PR out now. Thanks for doing this! 😄

@newoga
Copy link
Contributor

newoga commented Jan 31, 2016

Additional notes:

  • Remove list-divider export from lists/index.js
  • Shouldn't break anything, but since we're getting rid of DropDownIcon, we could probably delete this logic in toolbar-group.jsx: https://github.com/callemall/material-ui/blob/master/src/toolbar/toolbar-group.jsx#L188-L194
  • Since we've removed the show and dismiss imperative methods and their related onX props from snackbar.js, we should consider doing the same or deprecating the same on the date and time pickers
  • I'll admit, there was so much removed in left-nav and I didn't look at it super closely, but since it's fairly simple now, we could probably just test and make sure advertised functionality works

}
},

setValue(newValue) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This component's clearValue() method uses this function. We'll either have to remove that function too or implement it manually. Ideally we can just remove it but we never formally deprecated that function.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch!

Ideally we can just remove it but we never formally deprecated that function.

It was deprecated, calling clearValue was throwing the warning of setValue.

Copy link
Contributor

Choose a reason for hiding this comment

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

True! That makes sense, be gone clearValue()!

@newoga
Copy link
Contributor

newoga commented Feb 1, 2016

@oliviertassinari Alright, I gave this a quick over. I think we're close. After we tidy up the remaining issues, I'll test all the affected components one more time and then we should be done. I'm excited for removing code and cleaning up! Certainly should make it easier on contributors.

const menuItemElements = React.Children.map(children, (child) => {
const clone = React.cloneElement(child, {
onTouchTap: this._onMenuItemTouchTap.bind(this, index, child.props.value),
}, child.props.children);
Copy link
Member Author

Choose a reason for hiding this comment

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

That seems absurd to pass the children parameter. I'm gonna remove it, we will save some CPU cycles: https://github.com/facebook/react/blob/v0.14.7/src/isomorphic/classic/element/ReactElement.js#L252.

@oliviertassinari
Copy link
Member Author

Thanks for your feedback guys! I think that I have tackled all your points in my second commit.
It can be reviewed again.

* Callback function that is fired when the open state of the `LeftNav` is
* requested to be changed. The provided open argument determines whether
* the `LeftNav` is requested to be opened or closed. Also, the reason
* argument states why the `LeftNav` got closed or opend. It can be either
* `'clickaway'` for menuItem and overlay clicks, `'escape'` for pressing the
* `'clickaway'` for overlay clicks, `'escape'` for pressing the
* escape key and 'swipe' for swiping. For opening the reason is always `'swipe'`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Not your fault, but just noticed 'swipe' on this line also doesn't have the 'swipe' back ticks. Maybe we can just get it fixed here.

@oliviertassinari oliviertassinari changed the title [Core] Remove the deprecated API (and as many dead code as I could find) [Core] Remove the deprecated API of v0.14.x Feb 3, 2016
newoga added a commit to newoga/material-ui that referenced this pull request Feb 4, 2016
This commit removes the import of `mergeStyles` and `prepareStyles` from `utils/styles` for most components and files. The use of `mergeStyles` has been replaced with `Object.assign` and the `prepareStyles` function that is imported has been replaced with the `prepareStyles` implementation stored in context.

As of this commit, there are only two files importing from `utils/styles`:

1. `menus/menu-divider.jsx` (which is planned for removal in PR mui#3108)
2. `mixins/style-propable.js` (which many components are still using, but the removal of this mixin is tracked in mui#2852)

Signed-off-by: Neil Gabbadon <[email protected]>
@newoga
Copy link
Contributor

newoga commented Feb 4, 2016

@alitaheri I haven't had another full look at this since the first time. I can review again when I wake up but I remember feeling good about this from before. If you review and feel like its ready, you and @oliviertassinari can merge without me 👍

@alitaheri
Copy link
Member

Looking good @oliviertassinari. Thanks a lot 👍 👍 Feel free to merge this piece of art 😍 😍

@oliviertassinari
Copy link
Member Author

Awesome, I will rebase and merge 🚀. Thanks again for your feedbacks.

oliviertassinari added a commit that referenced this pull request Feb 4, 2016
[Core] Remove the deprecated API of v0.14.x
@oliviertassinari oliviertassinari merged commit 70b4e0f into mui:master Feb 4, 2016
@oliviertassinari oliviertassinari deleted the remove-deprecated-code branch February 4, 2016 16:06
@alitaheri
Copy link
Member

Awesome 👍 👍 🎈 🎉 🎈 😍

@newoga
Copy link
Contributor

newoga commented Feb 4, 2016

Awesome 👍 👍 🎈 🎉 🎈 😍

Agreed 👍 👍 I pulled this immediately. 😆

@oliviertassinari
Copy link
Member Author

I pulled this immediately

Same here, all my tests are green 🎉.

newoga added a commit to newoga/material-ui that referenced this pull request Feb 4, 2016
This commit removes the import of `mergeStyles` and `prepareStyles` from `utils/styles` for most components and files. The use of `mergeStyles` has been replaced with `Object.assign` and the `prepareStyles` function that is imported has been replaced with the `prepareStyles` implementation stored in context.

As of this commit, there are only two files importing from `utils/styles`:

1. `menus/menu-divider.jsx` (which is planned for removal in PR mui#3108)
2. `mixins/style-propable.js` (which many components are still using, but the removal of this mixin is tracked in mui#2852)

Signed-off-by: Neil Gabbadon <[email protected]>
@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.

5 participants