feat(Portal|Modal|Confirm): Better uncontrolled usage support #893
Conversation
Current coverage is 100% (diff: 100%)@@ master #893 diff @@
====================================
Files 137 137
Lines 2284 2298 +14
Methods 0 0
Messages 0 0
Branches 0 0
====================================
+ Hits 2284 2298 +14
Misses 0 0
Partials 0 0
|
| const { onCancel, onClose } = this.props | ||
| if (onClose) onClose(e, this.props) | ||
|
|
||
| // Only call onCancel if this was closed was some other way besides an |
| @@ -182,6 +200,15 @@ class Portal extends Component { | |||
| // Component Event Handlers | |||
| // ---------------------------------------- | |||
|
|
|||
| handlePortalClick = (e) => { | |||
| if (!this.props.closeOnCloseClick) return | |||
| if (!domUtils.closest(e.target, '[data-close]')) return | |||
There was a problem hiding this comment.
I'm not sure of an alternative proposal yet, but I don't think we should be leaving the virtual DOM to accomplish this. I see that this allows the user to define any element to be the "close" element and that this is important.
I'll give this some more thought and see what other proposals I can come up with. I think the goal should be that we do not read from the real DOM though.
There was a problem hiding this comment.
I think the main alternative to address the issues raised in the related issue are to make Modal an auto controlled component that handles "open" and have it implement
- closeIcon, and handle closing on click
- actions, which would be an array of buttons, with an "action" prop or something to determine if they are approve/deny/close buttons and handle them appropriately
What I like about the approach in this PR is:
- it's lightweight
- it stays consistent with to SUI-core (although they use classes, which are more brittle than data attributes imo)
- works regardless of how the user defines the underlying markup (children/shorthand)
- is usable outside of Modal, in any of our components that use portal (e.g. Adding a close button to a pop up) or in any user-defined components that use Portal directly
|
@levithomason - ping 😉 |
|
I'm hesitant on this one, I really don't think we should be reaching into the DOM for this. I've considered a few solutions, but I believe it is best to leave this to a controlled pattern until we solve it using the virtual DOM API. |
|
@davezuko do you have any wise ideas on parent-child communication in this regard? The only other thing I can think of is providing some Modal method that would return a wrapped "close" component. It would intercept the click and close the Modal, but this requires an obtuse API like: const { MyModal, Close } = Modal.createWithClose()
<MyModal>
<Close />
</MyModal>Where the parent-child relationship can be baked in when the create factory is invoked. We have a very similar problem for Sidebars which have a related Pusher component. The pattern is this need to have two components that can be placed anywhere in the tree but have a communication relationship established before they are rendered. |
|
Given we've merged the Modal closeIcon in #932 it would be good so see what this PR looks like after having those changes implemented here. This PR still has really great improvements outside of that. |
jeffcarbs
force-pushed
the
feature/modal-close
branch
from
3e3b192
to
0a1b784
Compare
|
Just rebased this. The updates to |
|
Blocked makes sense to me. Modal shorthand would be awesome :D |
|
We now support a close icon and there is a shorthand PR underway #1508. Should be unblocked here soon, finally! |
|
Modal shorthand is also now completed. Unblocking this one. |
|
Closing for #1599 |
Fixes #891
A few updates that I think will have a big impact in terms of usability of these components:
closeOnCloseClicktoPortalwhich makes it so that if an element with thedata-closeattribute within the portal is clicked, it will close the portal.Confirmcomponent so clicking confirm/cancel will close the modal (makes use ofcloseOnCloseClick). This enables uncontrolled usage and I've updated all of theConfirmexamples to make use of that where possible.closeIconprop toModalwhich renders an icon in the modal which, onClick, will trigger the modal to close (makes use ofcloseOnCloseClick). NOTE: With the default SUI styling, the icon appears outside of the modal. This is not an error on our side - the placement of the icon matches the SUI-core example and there are SUI CSS rules for.ui.modal > .closewhich intentionally put it out there. This seemed weird to me so I just wanted to call attention to it not being a bug: