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

Fix backButton handling in Toolbar #1

Merged
merged 1 commit into from Oct 31, 2017

Conversation

Projects
None yet
2 participants
@sstur
Member

sstur commented Oct 27, 2017

There's some code reformatting in here too, so this might look messy, but essentially I'm adding a single event listener for backPress.

@sstur sstur requested a review from AudyOdi Oct 27, 2017

@AudyOdi

Thank you for the changes @sstur, I have one additional idea tho. What if we add the back pressed listener if only the search is active and remove it whenever the search is inactive? So it will go like this in the constructor

const isSearchActive = this.props.isSearchActive || false;
if (isSearchActive) {
   BackAndroid.addEventListener('hardwareBackPress', this._onBackPress);
}

then in componentWillReceiveProps

if (!oldProps.isSearchActive && n ewProps.isSearchActive) { 
   // add listener
} else if (oldProps.isSearchActive && !newProps.isSearchActive) {
  // remove listener
}
? getBackButtonListener(this.onSearchCloseRequested)
: null;
const isSearchActive = this.props.isSearchActive || false;
BackAndroid.addEventListener('hardwareBackPress', this._onBackPress);

This comment has been minimized.

@AudyOdi

AudyOdi Oct 30, 2017

should we add this only if the search is active? because with this, the listener will always be added whenever the Toolbar is rendered right?

@AudyOdi

AudyOdi Oct 30, 2017

should we add this only if the search is active? because with this, the listener will always be added whenever the Toolbar is rendered right?

@sstur

This comment has been minimized.

Show comment
Hide comment
@sstur

sstur Oct 30, 2017

Member

I totally get what you're suggesting, but:

  1. There is confusing logic between props and state (both have an isSearchActive)
  2. I think the logic is easier if we add the listener ALWAYS and then just choose the action inside the listener based on the state/props. It does no harm to add a listener that returns false.
Member

sstur commented Oct 30, 2017

I totally get what you're suggesting, but:

  1. There is confusing logic between props and state (both have an isSearchActive)
  2. I think the logic is easier if we add the listener ALWAYS and then just choose the action inside the listener based on the state/props. It does no harm to add a listener that returns false.
@AudyOdi

This comment has been minimized.

Show comment
Hide comment
@AudyOdi

AudyOdi Oct 30, 2017

got it! I didn't know that it will execute all the event listeners when we attached more than 1 and about the return boolean. Let's ship it!

AudyOdi commented Oct 30, 2017

got it! I didn't know that it will execute all the event listeners when we attached more than 1 and about the return boolean. Let's ship it!

@AudyOdi AudyOdi added approved and removed ready for review labels Oct 30, 2017

@sstur sstur merged commit c66f952 into kodefox:master Oct 31, 2017

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