Add click outside functionality to the editor insert menu. #510
Conversation
There was a problem hiding this comment.
This works great but the menu is not being closed when we hit the + button a second time. Do we want to address this issue @jasmussen?
editor/components/inserter/index.js
Outdated
| @@ -16,7 +16,7 @@ class Inserter extends wp.element.Component { | |||
|
|
|||
| toggle() { | |||
There was a problem hiding this comment.
Should we rename this open?
There was a problem hiding this comment.
Let's first see if this becomes a toggle after I make a few changes.
editor/components/inserter/index.js
Outdated
| @@ -37,7 +37,7 @@ class Inserter extends wp.element.Component { | |||
| label={ wp.i18n.__( 'Insert block' ) } | |||
| onClick={ this.toggle } | |||
| className="editor-inserter__toggle" /> | |||
| { opened && <InserterMenu position={ position } onSelect={ this.close } /> } | |||
| { opened && <InserterMenu position={ position } onSelect={ this.close } onClickOutside={ this.close } /> } | |||
There was a problem hiding this comment.
Could we merge the onSelect and the onClickOutside prop into a single prop onClose that takes a nullable block parameter?
There was a problem hiding this comment.
The reason I created the second prop is in case there was an instance which we want a different actions for onSelect and onClickOutside.
I think we should keep these because I will probably use this to implement to toggle mentioned below.
| @@ -18,6 +19,10 @@ class InserterMenu extends wp.element.Component { | |||
| this.filter = this.filter.bind( this ); | |||
| } | |||
|
|
|||
| handleClickOutside( e ) { | |||
There was a problem hiding this comment.
CI is unhappy about the unused variable here (e).
|
I feel the button should remain a toggle. I'd guess there might have been some difficulty with event propagation and the outside click detection considering the button itself "outside" the menu. Maybe we should move the |
|
I believe bringing back the toggle is doable and can see a use case where the toggle is needed. I will try this today and let you know. |
| @@ -8,36 +8,55 @@ class Inserter extends wp.element.Component { | |||
| constructor() { | |||
| super( ...arguments ); | |||
| this.toggle = this.toggle.bind( this ); | |||
| this.close = this.close.bind( this ); | |||
| this.update = true; | |||
There was a problem hiding this comment.
Added to determine if we should run an update on the component (initial state is true).
| const { position } = this.props; | ||
|
|
||
| return ( | ||
| <div className="editor-inserter"> | ||
| <div className="editor-inserter" ref={ ( inserter ) => { |
There was a problem hiding this comment.
Added a ref to determine if the current button clicked (when menu is open) is the same button which opened the menu.
| toggle( e ) { | ||
| const toggle = this.inserter.getElementsByClassName( 'editor-inserter__toggle' ); | ||
|
|
||
| if ( 'undefined' !== typeof e.currentTarget && toggle[ 0 ] === e.currentTarget.activeElement ) { |
There was a problem hiding this comment.
Verifies that the toggle clicked is not a different toggle on the page.
|
I spent some time flushing this out today and believe it is now in working order. There are a few areas which seem a little tacky, but the functionality overall works as expected. I tried several methods, but settled on the following because all the other methods were failing due to the I added a few comments on some of the items. |
mtias
added
Framework
|
I still think there might be a simpler alternative where we simply bind the click outside logic to the parent See: 85bc8a6 In my testing, this seems to work just as well. The main downside is that there will be two global click handlers always present for each of the inserters, but they won't do anything unless the menu is currently open. Alternatively, maybe we could bind a click handler directly to the button and call |
|
@aduth yeah, I see what you mean now. I was trying to tie the handlers after the menu was opened, but your implementation looks much simpler. |
|
I have closed this in favor of #582. |
This fixes #376.