Conversation
|
Scheduled Jetpack release: January 12, 2021. Thank you for the great PR description! When this PR is ready for review, please apply the |
jeherve
left a comment
There was a problem hiding this comment.
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-builddependency tov6.3.0(#16845) is that thefocus-trapnpm 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...catchcalls).Some more context:
focusTrapwas 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-trapversion used perpackage.jsonwas^2.3.1.requires were updated to ESMimports by yours truly in Dashboard: Migrate CommonJS require()s to ESM imports #11677.focus-trapAPI 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.