Skip to content
This repository has been archived by the owner on Sep 13, 2020. It is now read-only.

Commit

Permalink
fixed the issue related to calling onChange multiple times
Browse files Browse the repository at this point in the history
  • Loading branch information
@alinz
alinz committed May 21, 2015
1 parent 855bcc1 commit ddc0253
Showing 1 changed file with 19 additions and 2 deletions.
21 changes: 19 additions & 2 deletions index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,12 @@ function shouldOpenMenu(dx: Number) {
function noop() {}

var SideMenu = React.createClass({
/**
* previous state of the menu, whether it is open or not
* @type {Boolean}
*/
prevState: false,

/**
* Current state of the menu, whether it is open or not
* @type {Boolean}
Expand Down Expand Up @@ -133,6 +139,17 @@ var SideMenu = React.createClass({
}
},

/**
* determins whether onChange on props needs to be called or not.
* @return {Void}
*/
isChanged: function () {
if (this.prevState != this.isOpen) {
this.prevState = this.isOpen;
this.props.onChange();
}
},

/**
* Open menu
* @return {Void}
Expand All @@ -143,7 +160,7 @@ var SideMenu = React.createClass({
this.updatePosition();
this.prevLeft = this.left;
this.isOpen = true;
this.props.onChange();
this.isChanged();
},

/**
Expand All @@ -156,7 +173,7 @@ var SideMenu = React.createClass({
this.updatePosition();
this.prevLeft = this.left;
this.isOpen = false;
this.props.onChange();
this.isChanged();
},

/**
Expand Down

3 comments on commit ddc0253

@Kureev
Copy link
Owner

@Kureev Kureev commented on ddc0253 May 26, 2015

Choose a reason for hiding this comment

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

I've though about this PR for a while and think that you can avoid extra complexity with new function and flag by adding following LoC:

if (this.isOpen) {
   this.props.onChange();
}

and

if (!this.isOpen) {
   this.props.onChange();
}

(before this.isOpen = true and this.isOpen = false)
instead of this.isChanged() and extra flag prevState. What do you think?

@alinz
Copy link
Contributor Author

@alinz alinz commented on ddc0253 May 26, 2015

Choose a reason for hiding this comment

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

Very smart, I like it. I think the less state the component has, the more predictable it will be.

@Kureev
Copy link
Owner

@Kureev Kureev commented on ddc0253 May 26, 2015

Choose a reason for hiding this comment

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

Great, so if you'll add this commit to PR, I'll merge it ;)

Please sign in to comment.