-
Notifications
You must be signed in to change notification settings - Fork 796
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
React Dashboard: Fix focusTrap import #17935
Conversation
Scheduled Jetpack release: January 12, 2021. Thank you for the great PR description! When this PR is ready for review, please apply the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to test well for me. Good catch!
I don't really know what this does 馃槵
I tested this by going to Jetpack > Dashboard on a connected site, clicking on the link to manage the connection in the Connection section, and once the modal was opened I tried to click into the wp-admin navigation menu on the left. As long as the trap is active, your clicks there won't do anything.
Thanks @kraftbj for merging! I wasn't able to do so earlier b/c the PHP 8.0 lint (which is clearly unrelated) blocked it for me 馃槄 |
All good. PHP 8 shouldn't have been a blocking check, but in either case, we should have some more PHP 8 joy landed pretty soon (the prod code is fine, we think, but the testing infra needed a bit to work). |
The PHP 8 linting should be blocking, IMO, since we're currently passing it and we don't want to regress. The problem wasn't actually in PHP 8 anyway, it's what we were discussing earlier at p1606835667387400-slack-CBG1CP4EN. |
Changes proposed in this Pull Request:
One of the build errors when attempting to upgrade the
@automattic/calypso-build
dependency tov6.3.0
(#16845) is that thefocus-trap
npm module doesn't have a default export symbol. (This is confirmed by its docs.)I assume that we're relying on a Babel plugin to add those default exports (mostly for CommonJS backwards compatibility), and the new calypso-build version would drop it. However, it even seems like
focus-trap
's API has long changed, so the current approach probably doesn't really work anyway (and just fails silently, since it's wrapped intry...catch
calls).Some more context:
focusTrap
was first introduced by Osk in Admin Page: Remove dops-components as dependency聽#8208 (as part of the big PR that brought dops-components into JP). At the time, thefocus-trap
version used perpackage.json
was^2.3.1
.require
s were updated to ESMimport
s by yours truly in Dashboard: Migrate CommonJS require()s to ESM imports聽#11677.focus-trap
API change apparently happened inv6.0.0
.Jetpack product discussion
N/A
Does this pull request change what data or activity we track or use?
Nah.
Testing instructions:
I don't really know what this does 馃槵
Proposed changelog entry for your changes:
React Dashboard: Fix focusTrap import.