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

[added] Don't focus after render if we don't want to #490

Merged
merged 1 commit into from
Sep 5, 2017

Conversation

imadha
Copy link
Contributor

@imadha imadha commented Sep 1, 2017

Fixes #306 .

Changes proposed:

  • Adds a shouldFocusAfterRender prop, defaults to true

Upgrade Path (for changed or removed APIs):

  • No upgrade necessary

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.

@imadha imadha force-pushed the do-not-focus-modal-by-default branch from 5c68f6e to dd609f3 Compare September 1, 2017 07:44
@coveralls
Copy link

Coverage Status

Coverage remained the same at 87.24% when pulling dd609f3 on imadha:do-not-focus-modal-by-default into 2adb45d on reactjs:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 87.24% when pulling dd609f3 on imadha:do-not-focus-modal-by-default into 2adb45d on reactjs:master.

@coveralls
Copy link

coveralls commented Sep 1, 2017

Coverage Status

Coverage remained the same at 87.24% when pulling dd609f3 on imadha:do-not-focus-modal-by-default into 2adb45d on reactjs:master.

@diasbruno
Copy link
Collaborator

Hi @imadha, can you update the documentation for this feature?

@diasbruno
Copy link
Collaborator

Forget the previous comment. Everything is ok.

@diasbruno diasbruno merged commit 93256e9 into reactjs:master Sep 5, 2017
@diasbruno
Copy link
Collaborator

Released v2.3.1. Thank you @imadha.

I think we need to update the documentation telling that it's not recommended to disable the focus due to some problems with accessibility and, perhaps, should be only used in case of using react-modal as a base component for other components. Any thoughts?

@diasbruno diasbruno self-requested a review September 5, 2017 19:23
Copy link
Collaborator

@diasbruno diasbruno left a comment

Choose a reason for hiding this comment

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

Approved.

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.

None yet

3 participants