Better props

[grabbou]

This pull request rewrites props-driven feature we have implemented in the past and makes side menu pure (meaning we can now implement shallowUpdate)

Somehow I wasn't happy with it because we were still using forceUpdate and keeping this.isOpen property. We were only respecting props changes whilst still keeping something similar to this.state.isOpen inside menu itself.

What was confusing? Well, changing prop - isOpen in parent component was resulting in onChange handler being called. And what if I changed that prop from inside onChange itself? I am in the loop! That should not be an issue anymore because of onContentPress function that has clearer purpose.

This is more an idea that needs to be discussed, but it essentially fixes above issues plus it removes disableGeastures in favour of onSwipe method, that is used to handle gesture changes now. If it's not defined, we assume that user does not want to handle gestures. It also makes it trivial to implement draggableBehaviour as described in #131 since there's no isOpen internal property.

Note: @Kureev - it's meant to be merged after #174 since I based on it to have better development workflow, but in case #174 is rejected, I'll rebase off the master.

[grabbou]

Last commit also makes overlay optional - in case one wants to have content still accessible and scrollable - simply does not register onContentPress handler. In that case, to close the menu, navbar button should be clicked.

[Kureev]

Hm, can you explain me how this one works? Not sure I understand it

[grabbou]

It's a function called when you drag menu with gesture. It's called with Boolean argument indicating whether menu is about to open or not. When you slide from left to open it, it will be called with true - menu is about to open. Now - if you want to open it, just update the state with that value (as per example). If you app is more advanced, you can embed some additional logic inside and sometimes set false under certain conditions.

[Kureev]

But we don't know if the menu is going to be opened or not, right? We can assert that menu is going to be closed or opened only at the moment when we release it (based on calculations we make).

Why do you think we need to give user a change if menu should be opened or not? I think it can be configured by the offsets user can setup.

Do you think it's a transparent behavior if function is not defined - assume that we don't want to enable gestures?

[grabbou]

But we don't know if the menu is going to be opened or not, right?

We know based on the calculations we do currently. It's just that instead of this.isOpen = someBooleanValue assignment we use a callback and pass that someBooleanValue up.

Do you think it's a transparent behavior if function is not defined - assume that we don't want to enable gestures?

That's what we do now. If that function is not defined, gestures are disabled.

Why do you think we need to give user a change if menu should be opened or not? I think it can be configured by the offsets user can setup.

Because that PR removes this.isOpen from component completely. Side menu has no internal state nor does not store any informations about whether it's open or not. It has to use a callback so that parent component updates state and this.props.isOpen reflects new position.

[Kureev]

Can you explain me how in this case (the case we don't have isOpen at all) we should approach menu opening using an external button (instead of swiping gesture)?

[grabbou]

Just like in examples/Basic :) onPress handler and parent component state update (or even redux call)

[Kureev]

So you want to force uses to write this animation by hand somewhere in reducers and run the full loop every frame?

[grabbou]

Not quite, let's discuss that on Skype tomorrow :D

[grabbou]

Hey other side menu users - WDYT? I believe I am not the only use who uses menu in production 💃

[esamattis]

Hey there! I'm using this. Just now upgraded to this branch because the current release does not respect the isOpen prop. The menu can be closed even if it is set to constant true.

But this branch works well with my Redux architecture where the menu state is kept inside the Redux store. No need for the menu to have it's own state (at least from the consumers perspective).

---

I actually think that all components should be initially developed as stateless components. The stateful version can be then easily implemented on top of it as a higher order component.

[esamattis]

I think I've found a bug with onSwipe.

Here openSideMenu and closeSideMenu props are the Redux action creators which toggle the menu state in the store and the isSideMenuOpen prop is the menu store value for the menu.

  handleSwipe(requestsOpening) {
      if (requestsOpening) {
          this.props.openSideMenu();
      } else {
          this.props.closeSideMenu();
      }
  },

   render() {
       var {isSideMenuOpen, isCategorySelected} = this.props;

       return (
           <View style={styles.container}>
               <SideMenu
                   menu={<Menu />}
                   isOpen={isSideMenuOpen}
                   onSwipe={this.handleSwipe}
                   onContentPress={this.props.closeSideMenu}
                   >

....

Now if you just slide the menu only few pixels and the onSwipe is not triggered the menu will get stuck between the states. I would expect it to be bounced back to the fully closed state. In react-native-page-swiper I implemented bounce back using a timer: fixt/react-native-page-swiper#8 I think something similar should work here too.

The bug appear also when closing the menu.

[esamattis]

Any updates?

[grabbou]

Unlikely @Kureev will merge this, but any advices / suggestions are welcome to make him change his mind :)

[esamattis]

Can you or @Kureev elaborate on that? I need a stateless side-menu implementation and if it's not something what's wanted for react-native-side-menu I'd like to know so I can move on.

[Kureev]

I think we'll discuss it today with Mike and I'll let you know.

[esamattis]

How it went? :)

[grabbou]

I don't know actually - would love to merge it and close other issues, but it's up to @Kureev. I am on this branch in my apps because of the mentioned loop issue.

[esamattis]

We are using this branch in production already :)