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

feat: add close button #76

Merged
merged 10 commits into from
Oct 11, 2018
Merged

feat: add close button #76

merged 10 commits into from
Oct 11, 2018

Conversation

frederickfogerty
Copy link
Contributor

Description

This PR adds a close button at the top-right of the screen. A close button seemed like a standard feature that this library should support. The close button can be disabled with showCloseButton: false in the configuration object.

This PR fixes #72

New Feature

  • [N/A] If this is a big feature with breaking changes, consider [opening an issue][issues] to discuss first. This is completely up to you, but please keep in mind that your PR might not be accepted.
  • Run unit tests to ensure all existing tests are still passing
  • Add new passing unit tests to cover the code introduced by your PR
  • Update the readme
  • Add some steps so we can test your cool new feature!
  • The PR title is in the conventional commit format: e.g. feat(<area>): added new way to do this cool thing #issue-number
  • Add your info to the contributors array in package.json!

Steps to Test

Review the changes to the tests.

Copy link
Contributor

@jayeb jayeb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor improvement requested, but otherwise looks good.

if (e && typeof e.preventDefault === "function") {
e.preventDefault();
}
if (this.settings.onClose) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably check that this.settings.onClose is actually a function before blindly calling it.

@frederickfogerty
Copy link
Contributor Author

@jayeb all changed, can you approve?

@frederickfogerty frederickfogerty merged commit 4548cbe into master Oct 11, 2018
@frederickfogerty frederickfogerty deleted the fred/close-button branch October 11, 2018 19:17
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 this pull request may close these issues.

Close icon
3 participants