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

[IconMenu] Removed props.ref call #3913

Merged
merged 1 commit into from
Apr 11, 2016
Merged

[IconMenu] Removed props.ref call #3913

merged 1 commit into from
Apr 11, 2016

Conversation

jakeboone02
Copy link
Contributor

  • PR has tests / docs demo, and is linted. (Feature had no demo before, so none was created for this PR)
  • Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
  • Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

Calls to props.ref will throw a warning in React v15.

This commit adds a prop to IconMenu called iconButtonRef that, if
provided, will be passed down to the child IconButton element as its ref.
The iconButtonRef element has been removed from the state object, since it
never changes after the component is initialized.

The new React warnings are discussed in this PR:
facebook/react#5744

@mbrookes mbrookes added PR: Needs Review external dependency Blocked by external dependency, we can’t do anything about it labels Apr 9, 2016
@mbrookes
Copy link
Member

mbrookes commented Apr 9, 2016

This is the relevant note: https://fb.me/react-special-props

@mbrookes mbrookes added this to the 0.15.0 Release milestone Apr 9, 2016
@jakeboone02
Copy link
Contributor Author

Thanks, @mbrookes, I meant to add that link, too, but forgot. Should I look at removing the calls to props.key as well (in a new PR)? There are three of them: one each in TableHeaderColumn, TableRow, and TableRowColumn.

@jakeboone02
Copy link
Contributor Author

Sorry for not waiting very long for a response, but #3918 was such a simple change I went ahead and did it.

@mbrookes
Copy link
Member

mbrookes commented Apr 9, 2016

Contributes to #3642.

@nathanmarks
Copy link
Member

@jakeboone02 quick Q:

Would this break any functionality where iconButtonRef is used if someone was previously relying on it? I'm looking at the close(reason, isKeyboard) function right now and was wondering...

We obviously need to get this compatible with react 15, I just want to be aware of anything that we're breaking so we can decide what to do about it.

@jakeboone02
Copy link
Contributor Author

I did a little more testing and from what I can tell, the feature never worked in the first place. If you pass ref="whatever" down to an IconMenu component, then access that component through this.refs.whatever and call on that component's refs, you still get 'iconButton' as the ref for the button. For example:

class IconMenuExampleSimple extends React.Component {
  componentDidMount() {
    console.log(this.refs.whatever.refs);
  }

  render() {
    return (
      <div>
        <IconMenu
          ref="whatever"
          iconButtonElement={<IconButton><MoreVertIcon /></IconButton>}
          anchorOrigin={{horizontal: 'left', vertical: 'top'}}
          targetOrigin={{horizontal: 'left', vertical: 'top'}}
        >
          <MenuItem primaryText="Refresh" />
...

logs this Object:

{
  iconButton: IconButton,
  iconMenuContainer: div
}

With the new code in place, passing iconButtonRef="whatever" results in this object:

{
  whatever: IconButton,
  iconMenuContainer: div
}

I suppose you could document it now that it works, but I don't think you need to include it in the list of breaking changes in 0.15.0.

@nathanmarks
Copy link
Member

@jakeboone02 thanks 👍

@nathanmarks
Copy link
Member

@mbrookes another one needed for react 15

@oliviertassinari
Copy link
Member

I did a little more testing and from what I can tell, the feature never worked in the first place.

In that case, I don't think that we should expose an API to change the ref.
Relying on anything else that the property API is often a bad idea an go against the React paradigme.
What do you think about removing iconButtonRef?

@jakeboone02
Copy link
Contributor Author

Yeah, I was thinking about that this morning and I agree. The only reason the ref is there is to "// Set focus on the icon button when the menu close" in the close(reason, isKeyboard) method. I can't imagine anyone actually needing to rename it, so I'll remove the option to do so.

@jakeboone02 jakeboone02 changed the title [IconMenu] Removed props.ref call, added iconButtonRef prop [IconMenu] Removed props.ref call Apr 11, 2016
@nathanmarks
Copy link
Member

@jakeboone02 if you're around, can you squash?

@jakeboone02
Copy link
Contributor Author

Sure. Never done a squash before so cross your fingers. ;)

Calls to props.ref will throw a warning in React v15.  This commit hard-codes the
value instead of reading props.ref.  The iconButtonRef element has been removed
from the state object, since it never changes after the component is initialized.

React warnings discussed in this PR:
facebook/react#5744
@nathanmarks
Copy link
Member

@jakeboone02 funnily enough I just got this error while booting up with react 15 😄

https://dl.dropboxusercontent.com/s/jypgxsv48cemif2/2016-04-11%20at%207.04%20PM.png

@nathanmarks nathanmarks merged commit e1544fd into mui:master Apr 11, 2016
@jakeboone02 jakeboone02 deleted the ref-key-props branch April 11, 2016 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external dependency Blocked by external dependency, we can’t do anything about it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants