[Added] Modal Close on right click at overlay #963
Conversation
|
We are going in the right direction, just made some comments. It must be the last change. |
|
Awesome!! 🎉 Can you include a test for this, please? |
|
@diasbruno I have added latest commit of example test case. Kindly let me know, if you have any suggestions. |
|
It looks great, but we also need to add an unit testing for this feature. Let me know if you need anything... |
|
Is there any unit testing already done? |
|
All unit tests are here https://github.com/reactjs/react-modal/tree/master/specs |
| @@ -52,6 +52,12 @@ import ReactModal from 'react-modal'; | |||
| /* Function that will be run when the modal is requested | |||
| to be closed (either by clicking on overlay or pressing ESC). | |||
| Note: It is not called if isOpen is changed by other means. */} | |||
|
|
|||
| onOverlayRightClick={ | |||
There was a problem hiding this comment.
I would propose onOverlayContextMenu as a better name for this prop, per @diasbruno's suggestion here.
We are passing this to React's onContextMenu event handler, which maps to the contextmenu event (see MDN docs):
The contextmenu event fires when the user attempts to open a context menu. This event is typically triggered by clicking the right mouse button, or by pressing the context menu key.
Some alternate ways to trigger the contextmenu event beyond right click:
ctrl+ click on MacOSshift+f10on Windowsfn+12on MacOS (with "Alternate Pointer Events" enabled)
...And some screen readers have their own triggers, too.
There was a problem hiding this comment.
Note that if we do decide to rename the prop, it may be worth updating the commits/PR title so that the wording in the changelog is accurate.
There was a problem hiding this comment.
fn+12 on MacOS (with "Alternate Pointer Events" enabled)
This one I didn't know. :)
There was a problem hiding this comment.
Yep, agree. Better to keep the names consistent.
| /* Function that will be run when the modal is requested | ||
| to be closed (either by right clicking on overlay or pressing ESC). | ||
| Note: It is not called if isOpen is changed by other means. */} |
There was a problem hiding this comment.
This comment is out of date given that 06c1c04 changed the prop to be a pass-through to onContextMenu instead of actually closing the modal. It is also not triggered by the esc key.
I would suggest a straightforward comment like:
Function that will be run when the context menu is triggered on the overlay (e.g., by right clicking or by pressing the context menu).
| handleOverlayRightClick = event => { | ||
| if(event.target == this.overlay) | ||
| { | ||
| event.preventDefault(); |
There was a problem hiding this comment.
We may want to allow the this.props.onOverlayRightClick function to call event.preventDefault() if they want to, for more flexibility. @diasbruno what do you think? 🤔
There was a problem hiding this comment.
Better for react-modal to not interfere in the event propagation. It would be better to let the user handles it. So, better to just forward the event if the handle is present.
| toggleModal = (event) => { | ||
| this.setState({ isOpen: !this.state.isOpen }); | ||
| }; | ||
| handleRightClick = (event) => { | ||
| this.setState({ isOpen: false }); | ||
| }; |
There was a problem hiding this comment.
Nit: missing a newline in between these two functions.
| isOpen={this.state.isOpen} | ||
| onOverlayRightClick={this.handleRightClick} | ||
| > | ||
| <h1>Click Right on the Overlay to close the modal</h1> |
There was a problem hiding this comment.
Nit: rephrase to "Right click on the overlay to close the modal"
| @@ -100,7 +101,8 @@ class Modal extends Component { | |||
| preventScroll: false, | |||
| parentSelector: () => document.body, | |||
| overlayElement: (props, contentEl) => <div {...props}>{contentEl}</div>, | |||
| contentElement: (props, children) => <div {...props}>{children}</div> | |||
| contentElement: (props, children) => <div {...props}>{children}</div>, | |||
| onOverlayRightClick: ()=>null | |||
There was a problem hiding this comment.
Nit: missing whitespace:
onOverlayRightClick: () => null
|
I'm sorry to interrupt, but I think it's awkward that the library provides click events. To implement the functionality of closing the modal when the user right-clicks, you can make use of |
|
@honeymaro It depends. You should use |
Fixes #[837].
Changes proposed:
-Added a prop of function type 'onOverlayRightClick' that calls a function when ever we right click on modal's overlay.
-The user can then change modal's state isOpen to false, if he wants to clocse the modal on right click at overlay
-
<Modal className="Modal__Bootstrap modal-dialog" isOpen={this.state.modalIsOpen} onRequestClose={this.handleModalCloseRequest} **onOverlayRightClick={this.handleModalCloseRequest}** >Acceptance Checklist:
CONTRIBUTING.md.