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

DOM element props should accept functions #10411

Closed
quisido opened this issue Feb 22, 2018 · 4 comments
Closed

DOM element props should accept functions #10411

quisido opened this issue Feb 22, 2018 · 4 comments
Labels
component: Popover The React component. new feature New feature or request
Milestone

Comments

@quisido
Copy link
Contributor

quisido commented Feb 22, 2018

For example the Popover component accepts an anchorEl prop which points to the DOM element to which to bind the popover.
However, if you are grabbing the DOM element with React's ref property, you must set a state or otherwise trigger re-render in order to get this DOM element to pass to Popover.
Triggering the re-render often re-triggers the ref, re-triggering the re-render, and repeat and repeat and repeat.
Importantly, nothing in the DOM is changing, so a re-render should not be necessary.

I feel like it would be much more efficient to not bother forcing a re-render if you could pass a function that returns the DOM element to anchorEl.

Expected Behavior

Not to have to trigger a re-render in order to pass a ref DOM element as a property.

Current Behavior

You have to trigger a re-render in order to pass a ref DOM element as a property.

Steps to Reproduce (for bugs)

  1. Create an element with a ref.
  2. Save the ref in the state. (TRIGGERS RE-RENDER)
  3. Pass the ref from the state to Popover.
  4. Popover appears over the ref.

Alternative

getSpanRef() {
  return this.spanRef;
}
render() {
  <div>
    <span ref={(ref) => { this.spanRef = ref; }} />
    <Popover anchorEl={this.getSpanRef} />
  </div>
}

Now whenever Popover needs to access anchorEl, it just calls this.props.anchorEl() instead of this.props.anchorEl.

It is also simple to check which type, a la: typeof this.props.anchorEl === 'function' ? this.props.anchorEl() : this.props.anchorEl

@oliviertassinari oliviertassinari added new feature New feature or request component: Popover The React component. labels Feb 22, 2018
@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 22, 2018

@CharlesStover Sure, why not. The Portal component already supports it:
https://github.com/mui-org/material-ui/blob/c0a712a986064c8c935a3063d9be2c54eb9b5bfc/src/Portal/Portal.js#L7-L10

Triggering the re-render often re-triggers the ref, re-triggering the re-render, and repeat and repeat and repeat.

You can avoid the ref rerender by providing the same function reference.

-<span ref={(ref) => { this.spanRef = ref; }} />
+<span ref={this.refHandler} />

@oliviertassinari oliviertassinari added this to the post v1.0.0 milestone Feb 22, 2018
@quisido
Copy link
Contributor Author

quisido commented Feb 22, 2018

How do you mean? The re-render has to occur in order for Popover to receive the new anchorEl prop, no? It was previously undefined and now it is that span DOM element. In order to set the Popover anchorEl prop to the span DOM element, I must trigger re-render. Otherwise, Popover will not appear, as anchorEl is undefined, even though ref has since triggered.

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 22, 2018

But you can avoid the ref rerender by providing the same function reference.

Like this, it's documented on React side.

@quisido
Copy link
Contributor Author

quisido commented Feb 22, 2018

The point isn't that ref is re-rendering, it's that ref has to re-render in order to get the new anchorEl prop to Popover.

This will not work unless a re-render takes place, to my knowledge:

<span ref={this.setSpanRef} />
<Popover anchorEl={this.spanRef} />

This will work regardless of a re-render, if Popover supports functions:

<span ref={this.setSpanRef} />
<Popover anchorEl={this.getSpanRef} />

oliviertassinari pushed a commit that referenced this issue Feb 23, 2018
* Added functional support for Popover (resolves #10411).
Fixed nested ternary operators.

* Replaced anchorEl component-wide with getter. (re #10420)

* Fix for `anchorEl` prop on a DOM element

* Attempt at resolving pages.

* isolation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: Popover The React component. new feature New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants