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

Adds null check for this.calendarMonthGrid ref #552

Merged
merged 1 commit into from
Jun 9, 2017

Conversation

majapw
Copy link
Collaborator

@majapw majapw commented Jun 8, 2017

Fix for #529

This should probably address #529. Having the ref be set in a constructor-bound method instead of in an inline arrow function should prevent the ref from being set to null unless it is unmounted. The extra null check should, in edge cases we are not thinking of, prevent the error itself from being thrown.

to: @ljharb @jherencia @johnryan

@majapw majapw added the semver-patch: fixes/refactors/etc Anything that's not major or minor. label Jun 8, 2017
ljharb
ljharb previously approved these changes Jun 8, 2017
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

LGTM, plus some comments

);
// eslint-disable-next-line react/no-find-dom-node
const calendarMonthGridDOMNode = ReactDOM.findDOMNode(this.calendarMonthGrid);
if (calendarMonthGridDOMNode) {
Copy link
Member

Choose a reason for hiding this comment

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

i'd actually wrap the findDOMNode in a truthy check for the ref; if it's nullary, there's no point in doing the findDOMNode

);
// eslint-disable-next-line react/no-find-dom-node
const calendarMonthGridDOMNode = ReactDOM.findDOMNode(this.calendarMonthGrid);
if (calendarMonthGridDOMNode) {
Copy link
Member

Choose a reason for hiding this comment

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

also here

@majapw majapw force-pushed the maja-add-null-check-for-calendarmonthgrid branch 2 times, most recently from d911d10 to 79daa93 Compare June 9, 2017 17:38
@majapw majapw force-pushed the maja-add-null-check-for-calendarmonthgrid branch from 79daa93 to 74868bf Compare June 9, 2017 20:35
@majapw majapw merged commit e07a22d into master Jun 9, 2017
@majapw majapw deleted the maja-add-null-check-for-calendarmonthgrid branch June 9, 2017 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch: fixes/refactors/etc Anything that's not major or minor.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants