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

Remove findDOMnode from Dropdown #844

Closed
interactivellama opened this issue Jan 5, 2017 · 2 comments · Fixed by #1557
Closed

Remove findDOMnode from Dropdown #844

interactivellama opened this issue Jan 5, 2017 · 2 comments · Fixed by #1557

Comments

@interactivellama
Copy link
Contributor

interactivellama commented Jan 5, 2017

I'm Running into issues with snapshot testing due to Button being an ES6 class extend of tooltip. Debugging Button/Tooltip. May need to refactor button a little. It's been on my mind for a while now.

It appears that Jest snapshots testing can't make any calls to focus or possibly all DOM references. This appears to be the issues: jestjs/jest#1353 (comment)

React’s test renderer module which is the default renderer for Jest snapshots does not implement a DOM, not even a JSDOM (which is what Enzyme does if not in a browser). Any components that use .focus(), portal mounts, or possibly any ReactDOM calls? cannot use Jest snapshot in its default implementation. Some folks are saying that newer versions of React/ReactDOM make TestRenderer work properly, but I’m not sure. Using Enzyme/JSDOM to get the HTML might be the way to go: https://github.com/adriantoine/enzyme-to-json or at least seems to be the “off the shelf” fix.

screen shot 2017-01-05 at 2 52 34 pm

Also, an interesting comment from last July https://twitter.com/dan_abramov/status/752950507690455040

screen shot 2017-01-05 at 3 30 41 pm

This project uses .findDOMnode() about 50 times (90% for focus)! I'm guessing we should be using props and callbacks like focusOnInput and then onFocusInput to reset the prop? Then, we'd put the .focus() on the actual node this.input through a ref.

Another options would be that callbacks get passed down as props and get called in the child and the component ref gets passed in.

This looks to be the preferred implementation. Use ref callbacks to pass child DOM nodes to parent:
reduxjs/react-redux#266 (comment)

On a related note onClickOutside needs to removed from components in order to have Jest test it. Pointing directly to components/datepicker/datepicker works.

@interactivellama interactivellama added this to the backburner milestone Jan 5, 2017
@interactivellama interactivellama changed the title Remove .findDOMnode from library Remove findDOMnode from library Jan 6, 2017
@futuremint
Copy link
Contributor

FYI: I'm eliminating these as I find them while fixing the ESLint errors.

@interactivellama interactivellama changed the title Remove findDOMnode from library Remove findDOMnode from Dropdown Dec 6, 2017
@interactivellama
Copy link
Contributor Author

This has been removed from all components except dropdown, modal which uses portals, and a few deprecated components.

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

Successfully merging a pull request may close this issue.

2 participants