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

[fixed] incorrect focusAfterRender on componentDidUpdate. #424

Merged
merged 3 commits into from
Jun 20, 2017
Merged

[fixed] incorrect focusAfterRender on componentDidUpdate. #424

merged 3 commits into from
Jun 20, 2017

Conversation

lindskogen
Copy link
Contributor

@lindskogen lindskogen commented Jun 17, 2017 •
edited by diasbruno
Loading

Maybe fixes #315.

Issue #315 seems to be a problem with inconsistent state when using react-modal with redux [1] (all errors seems to be reported by developer using redux).

It's not a proper fix since this behaviour was not reproduced, but it should be safe.

[1] - maybe a redux and/or component state async update. (?)

Changes proposed:

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.

@diasbruno
Copy link
Collaborator

Hi @lindskogen, thank you. There are a few lint problems. You can use npm run lint to see what is going on.

@coveralls
Copy link

coveralls commented Jun 18, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.05%) to 84.866% when pulling 57cbc40 on lindskogen:patch-1 into 21dc212 on reactjs:master.

@diasbruno diasbruno added this to the 2.1.0 milestone Jun 18, 2017
@diasbruno
Copy link
Collaborator

diasbruno commented Jun 18, 2017 •
edited
Loading

@lindskogen One last thing, can you rewrite the commit message to start with [fixed]? This is helpful to generate the changelog.

@diasbruno diasbruno changed the title Use bound ref functions [fixed] Use bound ref functions Jun 18, 2017
@diasbruno diasbruno added the bug label Jun 19, 2017
@lindskogen
Copy link
Contributor Author

@diasbruno I think that did it!

@coveralls
Copy link

Coverage Status

Coverage increased (+13.4%) to 84.866% when pulling d2b42d6 on lindskogen:patch-1 into 6c780ae on reactjs:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+13.4%) to 84.866% when pulling d2b42d6 on lindskogen:patch-1 into 6c780ae on reactjs:master.

@diasbruno
Copy link
Collaborator

I was trying to reproduce this issue (#315) and I think got it.
I've added some information on the issue.

Unfortunately, this won't fix the issue, but it will be merged as a recommendation/good practice.
Thank you for having a look on this.

@diasbruno @lindskogen
[fixed] check if the modal content is available when async update... (#1
a7bdfeb 
)

It seems that the using redux, we can get a async update in which can fail
when trying to focus the content of the modal.

This should be a temporary fix since the issue is hard to reproduce,
but safe for other applications.
diasbruno
diasbruno requested changes Jun 20, 2017
View reviewed changes
src/components/ModalPortal.js Outdated
@@ -122,7 +130,8 @@ export default class ModalPortal extends Component {
}

// Don't steal focus from inner elements
focusContent = () => (!this.contentHasFocus()) && this.content.focus();
focusContent = () =>
Copy link
Collaborator

@diasbruno diasbruno Jun 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, I've forgot to remove this trailing space after => .

@coveralls
Copy link

Coverage Status

Coverage increased (+13.4%) to 84.866% when pulling d8bdbe6 on lindskogen:patch-1 into 6c780ae on reactjs:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+13.4%) to 84.866% when pulling d8bdbe6 on lindskogen:patch-1 into 6c780ae on reactjs:master.

@diasbruno diasbruno changed the title [fixed] Use bound ref functions [fixed] incorrect focusContent when this.content is not available. Jun 20, 2017
@diasbruno diasbruno changed the title [fixed] incorrect focusContent when this.content is not available. [fixed] incorrect focusAfterRender on componentDidUpdate. Jun 20, 2017
@diasbruno
Copy link
Collaborator

@lindskogen thank you so much!

@diasbruno diasbruno merged commit 1676259 into reactjs:master Jun 20, 2017
@diasbruno diasbruno mentioned this pull request Jun 20, 2017
Closed
@diasbruno
Copy link
Collaborator

Released v2.0.6.

@kamalaknn kamalaknn mentioned this pull request Nov 16, 2022
Open
@snyk-bot snyk-bot mentioned this pull request Nov 29, 2022
Open
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
bug
Projects
None yet
Milestone
2.1.0
Development

Successfully merging this pull request may close these issues.

Undefined error in contentHasFocus method
3 participants
@lindskogen @diasbruno @coveralls