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

Components: Stop using `react-click-outside` as a dependency #16878

Merged
merged 1 commit into from Aug 8, 2019

Conversation

@gziolo
Copy link
Member

commented Aug 2, 2019

Description

Follow-up for #14851.

This PR addresses my comment from one of the PRs merged this week:

Unrelated question to this PR. Can we also refactor Modal component to stop using react-click-outside?

This allows us to get rid of react-click-outside dependency from the repository in favor of our own HOC withFocusOutside.

How has this been tested?

Open one of the modals from More Menu:

  • Block Manager
  • Keyboard Shortcuts
  • Options

Make sure it closes when you mouse-click outside of the modal.

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
@gziolo

This comment has been minimized.

Copy link
Member Author

commented Aug 2, 2019

By the way, when testing I discovered UI regression in Options modal:

Screen Shot 2019-08-02 at 12 01 10

/cc @mapk

Edit: I see it's tracked in #16860 👍

@jorgefilipecosta
Copy link
Member

left a comment

Works well in my tests 👍

@gziolo gziolo merged commit 8a052be into master Aug 8, 2019

3 of 42 checks passed

Filter opened Filter opened
Details
Filter opened Filter opened
Details
Filter opened Filter opened
Details
Filter opened Filter opened
Details
Filter opened Filter opened
Details
Filter opened Filter opened
Details
Filter opened Filter opened
Details
Filter opened Filter opened
Details
Filter opened Filter opened
Details
Filter opened Filter opened
Details
Filter opened Filter opened
Details
Filter opened Filter opened
Details
First Time Contributor First Time Contributor
Details
Assign Fixed Issues Assign Fixed Issues
Details
Filter opened Filter opened
Details
Filter opened Filter opened
Details
Filter opened Filter opened
Details
Filter opened Filter opened
Details
Filter opened Filter opened
Details
Filter opened Filter opened
Details
Filter opened Filter opened
Details
Filter opened Filter opened
Details
Filter opened Filter opened
Details
Filter opened Filter opened
Details
Filter opened Filter opened
Details
Filter opened Filter opened
Details
Milestone It Milestone It
Details
Milestone It Milestone It
Details
Milestone It Milestone It
Details
Milestone It Milestone It
Details
Milestone It Milestone It
Details
Milestone It Milestone It
Details
Milestone It Milestone It
Details
Milestone It Milestone It
Details
Milestone It Milestone It
Details
Milestone It Milestone It
Details
Milestone It Milestone It
Details
Milestone It Milestone It
Details
Milestone It Milestone It
Details
Filter opened Filter opened
Details
Filter opened Filter opened
Details
Travis CI - Pull Request Build Passed
Details

@gziolo gziolo deleted the remove/react-click-outside-dep branch Aug 8, 2019

@gziolo gziolo added this to the Gutenberg 6.3 milestone Aug 8, 2019

@noisysocks

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

Thanks for doing this @gziolo! ❤️

getdave added a commit that referenced this pull request Aug 15, 2019
Adds check to determine whether event occured within element trapping…
… focus

Previously there was no check to see whether the blur event occured within the wrapped component. If it occurs within then we do not want to trigger the `handleFocusOutside` callback because (by definition) the focus has not moved outside the wrapped element.

This may have been a regression introduced in #16878
getdave added a commit that referenced this pull request Aug 20, 2019
Check to ensure focus has intentionally left the wrapped component in…
… `withFocusOutside` HOC (#17051)

* Adds check to determine whether event occured within element trapping focus

Previously there was no check to see whether the blur event occured within the wrapped component. If it occurs within then we do not want to trigger the `handleFocusOutside` callback because (by definition) the focus has not moved outside the wrapped element.

This may have been a regression introduced in #16878

* Fix handling blur event when document is not actually focused

Previously we fixed the wrong problem. The actual issue is that previously we were handling document blur events as true “focus outside” events when in fact they should be ignored as if the document is not focused then presumably the user did not intent the blur.

* Update to test for document loss of focus scenario
donmhico added a commit to donmhico/gutenberg that referenced this pull request Aug 27, 2019
Check to ensure focus has intentionally left the wrapped component in…
… `withFocusOutside` HOC (WordPress#17051)

* Adds check to determine whether event occured within element trapping focus

Previously there was no check to see whether the blur event occured within the wrapped component. If it occurs within then we do not want to trigger the `handleFocusOutside` callback because (by definition) the focus has not moved outside the wrapped element.

This may have been a regression introduced in WordPress#16878

* Fix handling blur event when document is not actually focused

Previously we fixed the wrong problem. The actual issue is that previously we were handling document blur events as true “focus outside” events when in fact they should be ignored as if the document is not focused then presumably the user did not intent the blur.

* Update to test for document loss of focus scenario
gziolo added a commit that referenced this pull request Aug 29, 2019
Check to ensure focus has intentionally left the wrapped component in…
… `withFocusOutside` HOC (#17051)

* Adds check to determine whether event occured within element trapping focus

Previously there was no check to see whether the blur event occured within the wrapped component. If it occurs within then we do not want to trigger the `handleFocusOutside` callback because (by definition) the focus has not moved outside the wrapped element.

This may have been a regression introduced in #16878

* Fix handling blur event when document is not actually focused

Previously we fixed the wrong problem. The actual issue is that previously we were handling document blur events as true “focus outside” events when in fact they should be ignored as if the document is not focused then presumably the user did not intent the blur.

* Update to test for document loss of focus scenario
gziolo added a commit that referenced this pull request Aug 29, 2019
Check to ensure focus has intentionally left the wrapped component in…
… `withFocusOutside` HOC (#17051)

* Adds check to determine whether event occured within element trapping focus

Previously there was no check to see whether the blur event occured within the wrapped component. If it occurs within then we do not want to trigger the `handleFocusOutside` callback because (by definition) the focus has not moved outside the wrapped element.

This may have been a regression introduced in #16878

* Fix handling blur event when document is not actually focused

Previously we fixed the wrong problem. The actual issue is that previously we were handling document blur events as true “focus outside” events when in fact they should be ignored as if the document is not focused then presumably the user did not intent the blur.

* Update to test for document loss of focus scenario
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.