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

Switch from findDOMNode() to refs #100

Closed
tjbenneche opened this issue Dec 4, 2018 · 7 comments
Closed

Switch from findDOMNode() to refs #100

tjbenneche opened this issue Dec 4, 2018 · 7 comments
Milestone

Comments

@tjbenneche
Copy link

For components using container-query, snapshot tests fail because of the use of findDOMNode() in the library. This limitation is referenced in storybook's documentation here: This can be useful while testing react components which make use of the findDomNode API since they always fail with snapshot testing while using react-test-renderer

The invariant violation error I get is referenced in the core react repo here

It sounds like other projects have been able to avoid this issue by using refs instead of the findDOMNode API, for example: ianstormtaylor/slate#708

@ZeeCoder
Copy link
Owner

ZeeCoder commented Dec 4, 2018

Hmm, I'll do some digging.
The render method in is something like this:

render() {
    if (typeof this.props.children === "function") {
      return this.props.children(this.state.size);
    } else if (this.props.children) {
      return this.props.children;
    }

    return null;
  }

This means I can't simply just use the ref prop instead of findDOMNode.

I could, however add support for a new prop that accepts the element from the outside, in which case you'd be able to do render like this:

// `this.saveRef` saves the element as `this.element` to be used with the
// ContainerQuery component 
<ContainerQuery meta={meta} element={this.element}>
    <div ref={this.saveRef}>...</div>
</ContainerQuery>

In which case the component wouldn't call findDOMNode internally.

Would that work for you?

@tjbenneche
Copy link
Author

That's a cool idea. Any ideas how we could get this to work with the higher order function withContainerQuery? That's what I've been using

@ZeeCoder
Copy link
Owner

ZeeCoder commented Dec 5, 2018

I think there's a solution to that too, but I'd need to drop the wrappedComponentRef prop to make my job easier.

Don't suppose you use that prop?@tjbenneche

If that's fine with you, then I'll add it to v3.
If someone does need it in the future, they can either use the render prop version, or add support for it then in a PR 🤷‍♂️

@tjbenneche
Copy link
Author

Nope, I don't use that! So sounds good to me

@ZeeCoder
Copy link
Owner

ZeeCoder commented Dec 9, 2018

So I've experimented a little, but unfortunately I can't replace findDOMNode in withContainerQuery.
Reason being is that even if I use the "ref" prop on the wrapped component, I'll get the class, not the root dom node.

I could technically add an extra "element" argument to withContainerQuery, but when you call that function, you don't actually have access to the wrapped component's root dom element, so it doesn't help.

I'll experiment with the ContainerQuery prop, because I'm pretty sure that can be solved.

@ZeeCoder ZeeCoder added this to the 3.0.0 milestone Dec 9, 2018
@ZeeCoder
Copy link
Owner

ZeeCoder commented Dec 9, 2018

@tjbenneche the latest alpha should solve your issue:
https://github.com/ZeeCoder/container-query/releases

Here's an example on how to use the new ContainerQuery component:
https://codesandbox.io/s/wyl8molvl

@ZeeCoder
Copy link
Owner

This should now be fixed with v3

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

No branches or pull requests

2 participants