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

Update react-click-outside to 3.0 #10748

Merged

Conversation

blowery
Copy link
Contributor

@blowery blowery commented Oct 18, 2018

Description

Update react-click-outside to the latest version, the 3.0 line.

How has this been tested?

Verified that the popover still intercepts clicks outside the popover and closes the popover

Types of changes

Dependency update

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@blowery
Copy link
Contributor Author

blowery commented Oct 18, 2018

I think the problem here is that the package-lock.json I generated doing a local npm i is different from that which is generated by the npm i that happens as part of the test build. Therefore the package-lock is dirty and the ci build script fails.

Is there a standard way to generate the lock file?

Or should the builder use npm ci to avoid altering the lockfile?

@gziolo gziolo added this to the 4.2 milestone Oct 19, 2018
@gziolo
Copy link
Member

gziolo commented Oct 19, 2018

With all those local packages you probably need to do:

npm uninstall react-click-outside - to get rid of it from the lock file
npm i react-click-outside - to apply fresh

@blowery
Copy link
Contributor Author

blowery commented Oct 19, 2018

I got this to be happy by hand-removing the react-click-outside section

diff --git a/gutenberg-mobile b/gutenberg-mobile
--- a/gutenberg-mobile
+++ b/gutenberg-mobile
@@ -1 +1 @@
-Subproject commit 776bb23f7f19ef57b491f9708e9795b3a2ddb68e
+Subproject commit 776bb23f7f19ef57b491f9708e9795b3a2ddb68e-dirty
diff --git a/package-lock.json b/package-lock.json
index b2f7fa7d4..86afb9867 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -17105,14 +17105,6 @@
                 "prop-types": "^15.5.6"
             }
         },
-        "react-click-outside": {
-            "version": "2.3.1",
-            "resolved": "https://registry.npmjs.org/react-click-outside/-/react-click-outside-2.3.1.tgz",
-            "integrity": "sha1-MYc3698IGko7zUaCVmNnTL6YNus=",
-            "requires": {
-                "hoist-non-react-statics": "^1.2.0"
-            }
-        },
         "react-color": {
             "version": "2.13.4",
             "resolved": "https://registry.npmjs.org/react-color/-/react-color-2.13.4.tgz",

and then running npm i --package-lock-only in the root.

Previously I had been removing the package-lock entirely and running the above command.

If I do that after the current patch, I end up with a different layout in package-lock, with react-click-outside nested under components and missing the integrity key.

@blowery
Copy link
Contributor Author

blowery commented Oct 19, 2018

There's an additional dep for react-click-outside in the mobile repo. How do we coordinate changes like this across submodules?

@gziolo
Copy link
Member

gziolo commented Oct 19, 2018

There's an additional dep for react-click-outside in the mobile repo. How do we coordinate changes like this across submodules?

As long as it's in their own repo, we can only ping @hypest and ask to align versions :) We plan to have RN project inside this repo as a private package which would solve the issue.

@gziolo gziolo added the [Type] Code Quality Issues or PRs that relate to code quality label Oct 23, 2018
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, tested with the updated version of library 👍

@gziolo gziolo merged commit 61798e9 into WordPress:master Oct 23, 2018
@hypest
Copy link
Contributor

hypest commented Oct 24, 2018

There's an additional dep for react-click-outside in the mobile repo. How do we coordinate changes like this across submodules?

Assuming we're talking about the Gutenberg submodule in the mobile repo, it shouldn't be a problem to be "unaligned". It will get aligned "automatically" when we update the GB ref to a more recent one. The mobile app doesn't depend on react-click-outside anyway so, it won't be a problem.

antpb pushed a commit to antpb/gutenberg that referenced this pull request Oct 26, 2018
* Update react-click-outside to 3.0

* update package-lock
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants