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

Modal: Better uncontrolled usage support #891

Closed
jeffcarbs opened this issue Nov 18, 2016 · 3 comments
Closed

Modal: Better uncontrolled usage support #891

jeffcarbs opened this issue Nov 18, 2016 · 3 comments

Comments

@jeffcarbs
Copy link
Member

The SUI-core Modal supports various options for closing. See the "DOM Settings" section here: http://semantic-ui.com/modules/modal.html#settings

The following actions are called onClick of something matching the associated selector:

  close    : '.close, .actions .button',
  approve  : '.actions .positive, .actions .approve, .actions .ok',
  deny     : '.actions .negative, .actions .deny, .actions .cancel'
  • close just closes the modal
  • approve calls onApprove and closes the modal unless the return value from onApprove is false
  • deny calls onDeny and closes the modal unless the return value from onDeny is false.

This could be possible by attaching a click listener to the modal content and checking the currentTarget for one of these classes or perhaps a data-(close|approve|deny) attribute. This doesn't really feel very React-y, so open to other suggestions but I really think we should support something like this. Currently there's no way to:

  • use the Confirm component without managing open state yourself.
  • implement an X to close a Modal without wiring it to some state you're managing outside.

IMHO the most common use-cases for all of our components should be handled out of the box without the need to wrap things yourself.

One issue I see is that the modal technically doesn't manage open state, the Portal does. So we'd either have to:

  • implement this approve/deny functionality within Portal which feels kinda wrong
  • turn Modal into an AutoControlled component and have it manage it's own open state and pass it to Portal. From a user perspective, I don't think anything would change, we'd just have some duplicative functionality.

We could also forego the approve/deny functionality and add a closeOnCloseClick prop to Portal which would cause it to close if something with the data-close attribute was clicked within the Portal. I actually think that would be pretty reasonable.

Thoughts?

@levithomason
Copy link
Member

IMHO the most common use-cases for all of our components should be handled out of the box without the need to wrap things yourself.

Agreed!

We could also forego the approve/deny functionality and add a closeOnCloseClick prop to Portal which would cause it to close if something with the data-close attribute was clicked within the Portal.

I'm less fond of adding to the Portal, since these seem like specialized use cases rather than true Portal features, IMO.

What if instead, we provided some props for configuring the Modal buttons (actions)? There are only so many common "sets" of buttons you'd want, I think. For example, something like:

[primary] [secondary]
[positive] [negative]
[default]

If that is true, then we could provide shorthand props for individual buttons, or an array of butons:

<Modal leftAction={{ content: 'OK', primary: true }} rightAction='Cancel' />

// or

<Modal actions={[{ content: 'OK', primary: true }, 'Cancel']}

@jeffcarbs
Copy link
Member Author

@levithomason - check the related PR (#893). I think it solves the problem pretty elegantly and enables a lot more flexibility. Given that we don't currently support any shorthand for Modal, perhaps we could think about supporting specific action buttons if/when we implement Modal shorthand?

@layershifter
Copy link
Member

Closing for #1599.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants