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

Better props #175

Open
wants to merge 10 commits into
base: feature/better-props
Choose a base branch
from
Open

Better props #175

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
14dff14
Initial commit
grabbou Jan 8, 2016
8f93442
Use better name
grabbou Jan 8, 2016
d8509d7
Use the same method from Example
grabbou Jan 8, 2016
b90e0ff
Bring back old name
grabbou Jan 8, 2016
613e9dc
contentPress is required for the menu to close
grabbou Jan 8, 2016
9265966
Make overlay optional
grabbou Jan 8, 2016
e3e6903
Use proper english form
grabbou Jan 8, 2016
a659f11
Update readme
grabbou Jan 8, 2016
0ad70cf
Update readme
grabbou Jan 8, 2016
d9dccf2
Update readme example
grabbou Jan 8, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 23 additions & 6 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ class ContentView extends React.Component {
render() {
return (
<View style={styles.container}>
<TouchableOpacity onPress={this.props.onMenuButtonPress}>
<Text>Open menu</Text>
</TouchableOpacity>
<Text style={styles.welcome}>
Welcome to React Native!
</Text>
Expand All @@ -38,12 +41,26 @@ class ContentView extends React.Component {
}

class Application extends React.Component {
state = {
isOpen: false,
};

closeMenu() {
this.setState({ isOpen: false });
}

toggleMenu() {
this.setState({ isOpen: !this.state.isOpen });
}

render() {
const menu = <Menu navigator={navigator}/>;
const menu = <Menu navigator={navigator} />;

return (
<SideMenu menu={menu}>
<ContentView/>
<SideMenu
menu={menu}
onContentPress={() => this.closeMenu()}>
<ContentView onMenuButtonPress={() => this.toggleMenu()}/>
</SideMenu>
);
}
Expand All @@ -58,9 +75,9 @@ class Application extends React.Component {
- `edgeHitWidth` (Number) - Edge distance on content view to open side menu, defaults to 60
- `toleranceX` (Number) - X axis tolerance
- `toleranceY` (Number) - Y axis tolerance
- `disableGestures` (Bool) - Disable whether the menu can be opened with gestures or not
- `onStartShouldSetResponderCapture` (Function) - Function that accepts event as an argument and specify if side-menu should react on the touch or not. Check https://facebook.github.io/react-native/docs/gesture-responder-system.html for more details
- `onChange` (Function) - Callback on menu open/close. Is passed `isOpen` as an argument
- `onSwipe` (Function) - Function that handles gestures, receives boolean argument indicating whether menu should be opened or not based on the drag. When not defined, gestures are disabled
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

- `onStartShouldSetResponderCapture` (Function) - Function that accepts event as an argument and specify if side-menu should react on the touch or not. Check https://facebook.github.io/react-native/docs/gesture-responder-system.html for more details. Typically you would like to set `isOpen` value to the passed argument (see examples/Basic.js)
- `onContentPress` (Function) - Function that handles content press when menu is opened. Typically you would set isOpen to false inside it (see examples/Basic.js). When not present, content is accessible when menu is opened and there's no `touchToClose` behavior
Copy link
Owner

Choose a reason for hiding this comment

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

Why do you think we should make content accessible? Can you explain what motivated you to rewrite this behavior?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nothing made me to change it. I just think it's better to make that function optional, not required. It does not hurt anyone and makes SideMenu easier to set up. When there's no onContentPress - content is accessible and one can close menu with the same button he used to open it - just thinking it can save us few issues from people complaining they don't know how to close the menu. But it's not super important, so I am happy to revert it to be required, as previously.

Copy link
Owner

Choose a reason for hiding this comment

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

But it's a bit different from what I see: onContentPress looks like a replacement of onChange with a different behavior. I don't think we need to change it until we don't have any reason for it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The reasons for that are described in the PR description. Currently menu is still not props driven. Currently it uses forceUpdate and calls onChange so that you can update your component state with the correct value. That update causes yet another onChange event to be fired and you can end up in the loop.

Copy link
Owner

Choose a reason for hiding this comment

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

Currently, you don't have any access to the local state from the onChange callback. In the master branch you can't change any menu attribute but isOpen. It doesn't cause a problem that you describe.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Menu should not have any state nor that weird isOpen thing. We said that menu was going to be props driven which is not true currently, because it still keeps it's own isOpen. Why to have it when there's isOpen prop? To me, there's no need. If menu is prop driven, then there's no need for onChange - because what for? I just see the workflow proposed by this PR more declarative and pure.

To me, that's reasonable refactor, removing openMenu method and sticking entirely to props passed from the parent component, which is easier to reason about and gives better control.

Copy link
Owner

Choose a reason for hiding this comment

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

Well, openMenu is a private function and it isn't exposed to the users.

isOpen prop changes the state of the menu, I think it's logical.
onChange can be just removed I think. Or we use it as a callback after open/close animation to notify users about it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If it's entirely props driven, Or we use it as a callback after open/close animation to notify users about it. is not needed.

But why openMenu updates this.isOpen instead of using callback so that parent component that passed prop isOpen can update it manually and normal render cycle occurs?

I don't understand why onChange is called when I tap content when side menu is opened. There is no change actually (if we claim that is props driven). To me - onContentPress is way more logical, as in this PR.

Copy link
Owner

Choose a reason for hiding this comment

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

Can you explain why you think that onChange is not needed in this case? Sometimes people needs to know the exact moment when animation has been ended. Based on your feedback I think I can conclude that we can invoke some callback every time animated value changes (see https://facebook.github.io/react-native/docs/animations.html#input-events). It can be a good alternative to onChange, wdyt?

So literally, you want to make it possible to control an offset from the parent component? Like <SideMenu left={20} /> or something like this? If that's a case, we can approach it a bit diffirently: by dividing this component for "core" one - which will be a 100% props-driven without any local state. The only prop it'll take is a left prop. And make a wrapper for dragging and make a current behavior optional (like a top-level API). What do you think, would it cover both our cases? If changing a left property directly would be a case - you will be able to use a core component, if you want a more high-level api - use a different one. And both of them may be shipped inside a side-menu like react-native-side-menu and react-native-side-menu/core (or smth like that).

Copy link

Choose a reason for hiding this comment

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

When there's no onContentPress - content is accessible and one can close menu with the same button he used to open it - just thinking it can save us few issues from people complaining they don't know how to close the menu

@grabbou I need this exact thing right now. When i open the menu this.state.isOpen gets set correctly but when i try to close the menu the side menu press hijacks the event so this.state.isOpen is never changed when you close.

- `menuPosition` (String) - either 'left' or 'right', defaults to 'left'
- `animationFunction` (Function -> Object) - Function that accept 2 arguments (prop, value) and return an object:
- `prop` you should use at the place you specify parameter to animate
Expand Down
3 changes: 2 additions & 1 deletion examples/Basic/Basic.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,8 @@ module.exports = class Basic extends Component {
<SideMenu
menu={<Menu />}
isOpen={this.state.isOpen}
onChange={(isOpen) => this.updateMenuState(isOpen)}>
onSwipe={isOpen => this.updateMenuState(isOpen)}
onContentPress={() => this.toggle()}>
<View style={styles.container}>
<Text style={styles.welcome}>
Welcome to React Native!
Expand Down
49 changes: 14 additions & 35 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ class SideMenu extends Component {
* @type {Number}
*/
this.prevLeft = 0;
this.isOpen = props.isOpen;

this.state = {
width: deviceScreen.width,
Expand All @@ -61,38 +60,32 @@ class SideMenu extends Component {
});
}

componentWillReceiveProps(props) {
if (this.isOpen !== props.isOpen) {
this.openMenu(props.isOpen);
}
componentWillReceiveProps(newProps) {
const { isOpen, hiddenMenuOffset, openMenuOffset, } = newProps;
this.moveLeft(isOpen ? openMenuOffset : hiddenMenuOffset);
}

/**
* Determines if gestures are enabled, based off of disableGestures prop
* Determines if gestures are enabled, based off the presence of gesture handler
* @return {Boolean}
*/
gesturesAreEnabled() {
let { disableGestures, } = this.props;

if (typeof disableGestures === 'function') {
return !disableGestures();
}

return !disableGestures;
areGesturesEnabled() {
const { onSwipe, } = this.props;
return !!this.props.onSwipe;
}

/**
* Permission to use responder
* @return {Boolean}
*/
handleMoveShouldSetPanResponder(e: Object, gestureState: Object) {
if (this.gesturesAreEnabled()) {
if (this.areGesturesEnabled()) {
const x = Math.round(Math.abs(gestureState.dx));
const y = Math.round(Math.abs(gestureState.dy));

const touchMoved = x > this.props.toleranceX && y < this.props.toleranceY;

if (this.isOpen) {
if (this.props.isOpen) {
return touchMoved;
}

Expand Down Expand Up @@ -129,7 +122,7 @@ class SideMenu extends Component {
const offsetLeft = this.menuPositionMultiplier() *
(this.state.left.__getValue() + gestureState.dx);

this.openMenu(shouldOpenMenu(offsetLeft));
this.props.onSwipe(shouldOpenMenu(offsetLeft));
}

/**
Expand All @@ -150,29 +143,16 @@ class SideMenu extends Component {
this.prevLeft = newOffset;
}

/**
* Toggle menu
* @return {Void}
*/
openMenu(isOpen) {
const { hiddenMenuOffset, openMenuOffset, } = this.props;
this.moveLeft(isOpen ? openMenuOffset : hiddenMenuOffset);
this.isOpen = isOpen;

this.forceUpdate();
this.props.onChange(isOpen);
}

/**
* Get content view. This view will be rendered over menu
* @return {React.Component}
*/
getContentView() {
let overlay = null;

if (this.isOpen) {
if (this.props.isOpen && this.props.onContentPress) {
overlay = (
<TouchableWithoutFeedback onPress={() => this.openMenu(false)}>
<TouchableWithoutFeedback onPress={() => this.props.onContentPress()}>
<View style={styles.overlay} />
</TouchableWithoutFeedback>
);
Expand Down Expand Up @@ -220,10 +200,10 @@ SideMenu.propTypes = {
toleranceX: React.PropTypes.number,
toleranceY: React.PropTypes.number,
menuPosition: React.PropTypes.oneOf(['left', 'right', ]),
onChange: React.PropTypes.func,
onContentPress: React.PropTypes.func,
openMenuOffset: React.PropTypes.number,
hiddenMenuOffset: React.PropTypes.number,
disableGestures: React.PropTypes.oneOfType([React.PropTypes.func, React.PropTypes.bool, ]),
onSwipe: React.PropTypes.func,
animationFunction: React.PropTypes.func,
onStartShouldSetResponderCapture: React.PropTypes.func,
isOpen: React.PropTypes.bool,
Expand All @@ -236,7 +216,6 @@ SideMenu.defaultProps = {
openMenuOffset: deviceScreen.width * 2 / 3,
hiddenMenuOffset: 0,
onStartShouldSetResponderCapture: () => true,
onChange: () => {},
animationStyle: (value) => {
return {
transform: [{
Expand Down