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

Removed findDOMNode call and use refs #41

Conversation

fcsonline
Copy link

@fcsonline fcsonline commented Mar 28, 2017

More information here: #35

@fcsonline fcsonline force-pushed the feature/fix-for-find-dom-node branch from 9eec4dc to 924101b Compare March 28, 2017 10:29
@souporserious
Copy link
Owner

@fcsonline sorry just saw this, I'd like to not add an additional wrapping div. It seems like using refs is the only way to get testing to work with this component though 😕. Maybe we can allow people to pass the component they want measured as a prop? So it would be something like createElement(this.props.component, ). Thoughts?

@fcsonline
Copy link
Author

But then we are changing the api for this component...

Could we add a component wrapper and pass them as you say?

Like this:

render() {
    return (
      <MeasureWrapper ref={(c) => { this._node = c; }}>
          { filtered }
      </MeasureWrapper>
    );
  }

or

render() {
    return (
      <MeasureWrapper
          ref={(c) => { this._node = c; }}
          component={this.props.component}
      />
    );
  }

@souporserious
Copy link
Owner

souporserious commented Apr 3, 2017

Been thinking about this more and this would be a breaking change, or maybe just an addition, but what about a HoC called withDimensions or something that would give you access to the wrapped components dimensions through a dimensions prop? You would lose the callback though, maybe some other way that can be implemented 🤔

@souporserious
Copy link
Owner

souporserious commented Apr 3, 2017

Just did some poking around and came across this and looked at the eslint-react thread that was posted there as well. So it seems like the only path forward would be to use querySelector or wrap in an arbitrary div or user defined element. I think we would run into trouble if someone passed a custom component to this.props.component since it might not be a React element and could return another custom component, I might be wrong about that though.

@souporserious
Copy link
Owner

souporserious commented Apr 20, 2017

I think I finally landed on a solution!

So this would be using a HoC, but I think we can apply it to React Measure as well.

import React, { PropTypes, Component } from 'react'
import ResizeObserver from 'resize-observer-polyfill'

function withDimensions(MeasuredComponent) {
  return class ObservedComponent extends Component {
    static propTypes = {
      onLayout: PropTypes.func,
    }

    state = {
      dimensions: {
        width: -1,
        height: -1,
        top: -1,
        right: -1,
        bottom: -1,
        left: -1,
      },
    }

    componentDidMount() {
      this._resizeObserver = new ResizeObserver(() => this.measure())
      this._resizeObserver.observe(this._node)
      this.measure()
    }

    componentWillUnmount() {
      this.resizeObserver.disconnect(this._node)
    }

    measure() {
      const dimensions = this._node.getBoundingClientRect()

      this.setState({ dimensions })

      if (typeof this.props.onLayout === 'function') {
        this.props.onLayout(dimensions)
      }
    }

    _handleRef = component => {
      this._node = component

      if (typeof this.props.getRef === 'function') {
        this.props.getRef(component)
      }
    }

    render() {
      const { getRef, onLayout, ...props } = this.props
      return (
        <MeasuredComponent
          {...props}
          getRef={this._handleRef}
          dimensions={this.state.dimensions}
        />
      )
    }
  }
}

export default withDimensions

so the biggest difference is just requiring the user to pass the getRef callback down properly.

usage for the HoC looks like this:

const MeasuredDiv = withDimensions(({ getRef, dimensions, ...props }) => (
  <div ref={getRef} {...props} />
))

I'll try and take a stab at it for React Measure ASAP. Feel free to take a pass at it if you would like to.

@souporserious
Copy link
Owner

Finally landed on something 😅 thanks for your initial work! It inspired the latest release 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants