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

Set Modals to only close with ESC key or Cancel & X buttons #5823

Merged
merged 3 commits into from Apr 3, 2019

Conversation

@kyleknighted
Copy link
Contributor

commented Apr 1, 2019

Description

Set default for Modal component to not automatically close when outside of component is clicked.

Motivation and Context

Closes #5803
https://react-bootstrap.github.io/components/modal/#modals-props

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
@edmundoa
Copy link
Member

left a comment

LGTM 👍

@bernd
Copy link
Member

left a comment

I think we should come up with a plan on what we do with other modals in the product (including enterprise) that don't use the BootstrapModalWrapper component before we merge this.

  • Are other modals behaving the same if we merge this?
  • If not, do we want to adjust their behavior to match the new one?
  • Are there reasons for other modals behaving differently?

@bernd bernd requested review from bernd and removed request for bernd Apr 3, 2019

@bernd
bernd approved these changes Apr 3, 2019
@edmundoa

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

@bernd we discussed how to proceed with this issue and @dennisoelkers volunteered to investigate how to better unify the Modals in enterprise (Graylog2/graylog-plugin-enterprise#913).
We also consider this PR is an improvement over the current behaviour, I think this PR can be merged, unless you have any other concerns about it :)

@edmundoa edmundoa merged commit 3a6bf9f into master Apr 3, 2019

4 checks passed

ci-web-linter Jenkins build graylog-pr-linter-check 3500 has succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
graylog-project/pr Jenkins build graylog-project-pr-snapshot 3577 has succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@edmundoa edmundoa deleted the issue-5803 branch Apr 3, 2019

bernd added a commit that referenced this pull request May 23, 2019
Set Modals to only close with ESC key or Cancel & X buttons (#5823)
* Set Modals to only close with ESC key or Cancel & X buttons

* Update snapshot

(cherry picked from commit 3a6bf9f)
dennisoelkers added a commit that referenced this pull request Jul 18, 2019
Set Modals to only close with ESC key or Cancel & X buttons (#5823) (#…
…5966)

* Set Modals to only close with ESC key or Cancel & X buttons

* Update snapshot

(cherry picked from commit 3a6bf9f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.