-
Notifications
You must be signed in to change notification settings - Fork 70
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
Code base needs to be modernized to drop findDOMNode and string refs #77
Comments
- All dependencies have been updated to their very latest versions. Some were dropped in favor of more modern ones (in particular, related to Babel 6.x -> 7.x). - Added ESLint Prettier rules. - Updated Prettier to 2.x, configured options, formatted code. - Fixed a bug that was causing a crash expecting `includes()` to be a method on `specifiedFocusTrapOptions`. - Added babel-jest to remove the need to first build the bundle prior to running tests. - Added format-check package script to check for formatting issues. - Added format and lint checks to `npm test`, along with unit tests. - Some lint errors were disabled, deferred to #77
Any updates on this? |
No progress. Is this causing you trouble? |
Disabling by default for now because `focus-trap-react` is throwing StrictMode errors: focus-trap/focus-trap-react#77 Also worth nothing that currently StrictMode does not work in the Docs tab (only Canvas): storybookjs/storybook#17880
Disabling by default for now because `focus-trap-react` is throwing StrictMode errors: focus-trap/focus-trap-react#77 Also worth nothing that currently StrictMode does not work in the Docs tab (only Canvas): storybookjs/storybook#17880
@m7kvqbe1 Thanks for letting me know. As it stands, this would be the next priority to work on for this project, provided nothing more urgent arises in the meantime. I can't make promises on a timeline, however. Just hopefully "soon". Issues on focus-trap and tabbable could also detract from this, but as of this past Thursday, I think I finally got all 3 in a good place again. 😅 |
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.
Hoping #710 will fix this. |
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.
Look for a fix to this (I hope!!) in v9.0.0 being published right now. |
The following lint issues had to be disabled in order to maintain long backward compatibility to React 16.0:
TL/DR: Using callback refs should clear all these up.
Sooner or later, however, these issues will have to be rectified in order for the code to be compatible with the next major release of React, at which point, we'll have to drop support for < React 16.3 which is when
React.createRef()
and callback refs were introduced (AFAICT).The text was updated successfully, but these errors were encountered: