Skip to content

Commit

Permalink
[#77] Finally drop use of findDOMNode() (#710)
Browse files Browse the repository at this point in the history
Fixes #77
Fixes #704

For the longest time, since before I took over, we were filtering
specified containers through `ReactDOM.findDOMNode()`, but why?

The configured trap containers (via the `containerElements` prop)
 must be elements already, which means they must already be rendered,
which means there's no point in passing them through `findDOMNode()`
to find an underlying DOM element because they aren't React elements
in the first place.

The removal of this call should mean that React Strict Mode will
finally be OK with focus-trap-react.

Note it's still possible to render a focus trap with a single React
element child. That works fine, and never needed `findDOMNode()`
anyway. We were already using a callback ref to get its element
to then auto-configure that element as the single container for
the focus trap.

Just to be sure, however, this will be released as a new __major__
version.
  • Loading branch information
stefcameron committed Jun 10, 2022
1 parent a927393 commit 4a77d87
Show file tree
Hide file tree
Showing 3 changed files with 11 additions and 19 deletions.
5 changes: 5 additions & 0 deletions .changeset/shy-terms-fold.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'focus-trap-react': major
---

Stop using the infamous `findDOMNode()` on provided `containerElements`. There seems to have been no good reason for this as this prop, if specified, is already required to be an array of HTMLElement references, which means these nodes have already been rendered (if they were once React elements). There appears to have been no remaining need for this API. Furthermore, the minimum supported version of React is now 16.3 as it technically has been for a while now since that is the version that introduced callback refs, which we've been using for quite some time now (so this bump shouldn't cause any ripples).
4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@
},
"peerDependencies": {
"prop-types": "^15.8.1",
"react": ">=16.0.0",
"react-dom": ">=16.0.0"
"react": ">=16.3.0",
"react-dom": ">=16.3.0"
}
}
21 changes: 4 additions & 17 deletions src/focus-trap-react.js
Original file line number Diff line number Diff line change
@@ -1,14 +1,8 @@
const React = require('react');
const ReactDOM = require('react-dom');
const PropTypes = require('prop-types');
const { createFocusTrap } = require('focus-trap');
const { isFocusable } = require('tabbable');

// TODO: These issues are related to older React features which we'll likely need
// to fix in order to move the code forward to the next major version of React.
// @see https://github.com/davidtheclark/focus-trap-react/issues/77
/* eslint-disable react/no-find-dom-node */

class FocusTrap extends React.Component {
constructor(props) {
super(props);
Expand Down Expand Up @@ -277,18 +271,11 @@ class FocusTrap extends React.Component {

setupFocusTrap() {
if (!this.focusTrap) {
const focusTrapElementDOMNodes = this.focusTrapElements.map(
// NOTE: `findDOMNode()` does not support CSS selectors; it'll just return
// a new text node with the text wrapped in it instead of treating the
// string as a selector and resolving it to a node in the DOM
ReactDOM.findDOMNode
);

const nodesExist = focusTrapElementDOMNodes.some(Boolean);
const nodesExist = this.focusTrapElements.some(Boolean);
if (nodesExist) {
// eslint-disable-next-line react/prop-types -- _createFocusTrap is an internal prop
this.focusTrap = this.props._createFocusTrap(
focusTrapElementDOMNodes,
this.focusTrapElements,
this.internalOptions
);

Expand Down Expand Up @@ -378,7 +365,7 @@ class FocusTrap extends React.Component {
);
}

const composedRefCallback = (element) => {
const callbackRef = (element) => {
const { containerElements } = this.props;

if (child) {
Expand All @@ -395,7 +382,7 @@ class FocusTrap extends React.Component {
};

const childWithRef = React.cloneElement(child, {
ref: composedRefCallback,
ref: callbackRef,
});

return childWithRef;
Expand Down

0 comments on commit 4a77d87

Please sign in to comment.