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.

Sign up for GitHub

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

Jump to bottom

[added] shouldCloseOnEsc prop #487

Merged
merged 1 commit into from
Oct 11, 2017
Merged

[added] shouldCloseOnEsc prop #487

merged 1 commit into from
Oct 11, 2017

Conversation

spen
Copy link
Contributor

@spen spen commented Aug 27, 2017 •
edited
Loading

Fixes #481.

Changes proposed:

  • Add a new prop: shouldCloseOnEsc
  • Allow disabling of closing the modal on esc press.

Acceptance Checklist:

  • All commits have been squashed to one.
  • The commit message follows the guidelines in CONTRIBUTING.md.
  • Documentation (README.md) and examples have been updated as needed.
  • If this is a code change, a spec testing the functionality has been added.
  • If the commit message has [changed] or [removed], there is an upgrade path above.

@spen spen mentioned this pull request Aug 27, 2017
Merged
@spen spen force-pushed the update/add-prop-to-disable-close-on-esc branch from 90e0065 to f94481c Compare August 27, 2017 16:00
@coveralls
Copy link

Coverage Status

Coverage remained the same at 87.24% when pulling f94481c on spen:update/add-prop-to-disable-close-on-esc into b908042 on reactjs:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 87.24% when pulling f94481c on spen:update/add-prop-to-disable-close-on-esc into b908042 on reactjs:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 87.24% when pulling bb59386 on spen:update/add-prop-to-disable-close-on-esc into b908042 on reactjs:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 87.24% when pulling bb59386 on spen:update/add-prop-to-disable-close-on-esc into b908042 on reactjs:master.

@diasbruno
Copy link
Collaborator

diasbruno commented Aug 28, 2017 •
edited
Loading

@spen Can you update the commit message to include [add], please? Since this addition is backwards compatible, this will bump a minor version.

@spen
Copy link
Contributor Author

spen commented Aug 28, 2017

@diasbruno sure thing! I'll squish it all done while I'm at it.
Does everything else look good bar the commits?

@spen spen force-pushed the update/add-prop-to-disable-close-on-esc branch from bb59386 to 2a4f9fc Compare August 28, 2017 17:13
@coveralls
Copy link

Coverage Status

Coverage remained the same at 87.24% when pulling 2a4f9fc on spen:update/add-prop-to-disable-close-on-esc into b908042 on reactjs:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 87.24% when pulling 2a4f9fc on spen:update/add-prop-to-disable-close-on-esc into b908042 on reactjs:master.

@spen spen force-pushed the update/add-prop-to-disable-close-on-esc branch from 2a4f9fc to 7b82fa6 Compare August 28, 2017 17:29
@coveralls
Copy link

coveralls commented Aug 28, 2017 •
edited
Loading

Coverage Status

Coverage remained the same at 87.24% when pulling 7b82fa6 on spen:update/add-prop-to-disable-close-on-esc into b908042 on reactjs:master.

@spen
Copy link
Contributor Author

spen commented Aug 28, 2017

Squished & amended :)
I also added an entry in docs/README.md.

P.S. Sorry for all the coveralls triggers! 😆

@diasbruno
Copy link
Collaborator

@spen It looks good. Tomorrow, I'll finish to review this PR.

@diasbruno diasbruno changed the title Add shouldCloseOnEsc prop [added] shouldCloseOnEsc prop Sep 1, 2017
@diasbruno
Copy link
Collaborator

diasbruno commented Sep 5, 2017 •
edited
Loading

@spen Can you rebase your branch, please? I'll get this merged.

Perhaps, we need to update the documentation to warn about the accessibility issue if using this flag.

@spen
Copy link
Contributor Author

spen commented Sep 5, 2017

@diasbruno That makes a lot of sense, I'll add & rebase :)

@spen
Copy link
Contributor Author

spen commented Oct 11, 2017

Sorry for the hiatus here @diasbruno & thanks for your patience - I've had lots of other priorities lately!
I'm looking at making some final changes to push this forward soon :)

@diasbruno
Copy link
Collaborator

No problem.

@spen spen force-pushed the update/add-prop-to-disable-close-on-esc branch from 7b82fa6 to 06ff57d Compare October 11, 2017 13:07
@spen
Copy link
Contributor Author

spen commented Oct 11, 2017

Thanks Dias,
I've added the following in docs/README.md:

/*
   Boolean indicating if pressing the esc key should close the modal
   Note: By disabling the esc key from closing the modal you may introduce an accessibility issue.
 */
shouldCloseOnEsc={true}

Please do let me know if you'd like any changes to the wording or for extra doc coverage elsewhere in the project :)

@diasbruno
Copy link
Collaborator

Look good. I'll get this merged today.

@diasbruno
Copy link
Collaborator

diasbruno commented Oct 11, 2017 •
edited
Loading

@spen A few things before merge:

  • Squash the commits.
  • Add [fixed] to the commit message.
  • ...and rebase if necessary.

@spen spen force-pushed the update/add-prop-to-disable-close-on-esc branch 2 times, most recently from 44bc576 to 9ccab60 Compare October 11, 2017 21:51
@coveralls
Copy link

coveralls commented Oct 11, 2017 •
edited
Loading

Coverage Status

Coverage remained the same at 86.34% when pulling 9ccab60 on spen:update/add-prop-to-disable-close-on-esc into d98f091 on reactjs:master.

diasbruno
diasbruno requested changes Oct 11, 2017
View reviewed changes
src/components/ModalPortal.js Outdated
@@ -18,6 +18,7 @@ const ESC_KEY = 27;

export default class ModalPortal extends Component {
static defaultProps = {
shouldCloseOnEsc: true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can move this prop to Modal.js and modal will pass to the portal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well spotted!
I've moved the default prop value :)

@spen
[fixed] Add shouldCloseOnEsc prop
6991467 
shouldCloseOnEsc: Add prop and logical check to keyHandler

shouldCloseOnEsc: Add to docs/README.md
@spen spen force-pushed the update/add-prop-to-disable-close-on-esc branch from 9ccab60 to 6991467 Compare October 11, 2017 22:04
@coveralls
Copy link

Coverage Status

Coverage remained the same at 86.34% when pulling 6991467 on spen:update/add-prop-to-disable-close-on-esc into d98f091 on reactjs:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 86.34% when pulling 6991467 on spen:update/add-prop-to-disable-close-on-esc into d98f091 on reactjs:master.

@diasbruno diasbruno self-requested a review October 11, 2017 22:12
@spen
Copy link
Contributor Author

spen commented Oct 11, 2017

Thanks heaps for helping with these finishing touches Bruno :)

@diasbruno diasbruno merged commit 1d495a6 into reactjs:master Oct 11, 2017
@spen spen deleted the update/add-prop-to-disable-close-on-esc branch October 12, 2017 08:07
@diasbruno
Copy link
Collaborator

Released v3.0.2.

@spen
Copy link
Contributor Author

spen commented Oct 14, 2017

Cheers Bruno! :)

@Andersos
Copy link

Thanks for fixing this! Just started using this and it works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@diasbruno diasbruno diasbruno approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@spen @coveralls @diasbruno @Andersos