-
Notifications
You must be signed in to change notification settings - Fork 298
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
Some fixes and new props #115
Some fixes and new props #115
Conversation
Hey! thanks for that, this is very good stuff! I will look into it next week and try it out on apps! Cheers |
docs/EXAMPLES.md
Outdated
@@ -29,7 +29,7 @@ yarn start | |||
```bash | |||
cd examples/react-native-navigation | |||
yarn ios (or android) | |||
cd ../.. | |||
cd ../ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The package json containing the watch script is on the root of the project, so it's correct to be 2 levels higher
src/index.tsx
Outdated
@@ -186,18 +190,23 @@ export class Modalize<FlatListItem = any, SectionListItem = any> extends React.C | |||
const { withHandle } = this.props; | |||
|
|||
if (!withHandle) { | |||
return 20; | |||
return 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By removing the 20px, the modal is now stuck to the top of the screen without margin. It was intentional to have this space when the handle is not shown. Even though, I'm not again a props to change this margin
src/index.tsx
Outdated
const { modalHeight, headerHeight, footerHeight } = this.state; | ||
const contentViewHeight: ViewStyle[] = []; | ||
|
||
if (!adjustToContentHeight) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I tried to open the simple content example on Android, and press inside the input, the modal went back down, and wasn't focus on the input, and I couldn't scroll to it. For iOS it was all good
src/index.tsx
Outdated
style={{ marginBottom }} | ||
enabled={enabled} | ||
> | ||
<Animated.View style={!adjustToContentHeight ? s.flex1 : null}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the issue related to the modal that goes down on Android is because of this keyboard avoiding view getting remove. But I just tried it quickly, so not sure
src/styles.ts
Outdated
@@ -90,4 +90,8 @@ export default StyleSheet.create({ | |||
|
|||
backgroundColor: 'rgba(0, 0, 0, 0.65)', | |||
}, | |||
|
|||
flex1: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you rename that into children
?
@jeremybarbet please check the next changes, changed a way to handle the modal height |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know the keyboard behavior was already broken on Android, but after trying it out on the examples, it still doesn't translate the modal when the keyboard appears.
The rest of the changes looks good to me! Just let me know if you can fix the keyboard issue on Android or need a bit of help 👍🏻
@@ -116,34 +122,30 @@ export class Modalize<FlatListItem = any, SectionListItem = any> extends React.C | |||
isVisible: false, | |||
showContent: true, | |||
overlay: new Animated.Value(0), | |||
modalHeight, | |||
modalHeight: props.adjustToContentHeight ? undefined : modalHeight, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modalHeight: props.adjustToContentHeight ? undefined : modalHeight, | |
modalHeight: props.adjustToContentHeight ? 0 : modalHeight, |
From what I see, you check all the time (at least from what I saw) modalHeight || 0
in the following file. Wouldn't be faster to define it at first? and you wouldn't have to do modalHeight || 0
anywhere else
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modal height undefined
on the first mount works as height: auto
, if we set the height to 0 onLayout will return height:0
every time
@@ -342,35 +364,6 @@ export class Modalize<FlatListItem = any, SectionListItem = any> extends React.C | |||
); | |||
}; | |||
|
|||
private onContentViewChange = (keyboardHeight?: number): void => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice to get rid of that 👍
src/index.tsx
Outdated
return ( | ||
<View style={s.component} onLayout={onLayout} pointerEvents="box-none"> | ||
<View style={s.component} pointerEvents="box-none"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say that the View is not necessary here anymore since you removed the onLayout props
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, but I was afraid to clean up, I'll try
src/index.tsx
Outdated
const keyboardAvoidingViewProps: KeyboardAvoidingViewProps = { | ||
keyboardVerticalOffset: keyboardAvoidingOffset, | ||
behavior: keyboardAvoidingBehavior || 'padding', | ||
enabled, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enabled, | |
isIos, |
Can you straight isIos
and remove enabled variable
src/index.tsx
Outdated
style: [s.modalize__content, this.modalizeContent, modalStyle], | ||
}; | ||
|
||
if (!isIos) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you not running the function on ios?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because if pass onLayout prop to KAW on iOS, it overrides onLayout logic in KAW and it stops working(RN bug) https://github.com/facebook/react-native/blob/8553e1acc4195479190971cc7a3ffaa0ed37a5e0/Libraries/Components/Keyboard/KeyboardAvoidingView.js#L119
could you give me a screenshot or video, I tested examples on both platforms, and it worked on both |
@jeremybarbet videos |
Here you go: https://share.getcloudapp.com/nOum7QAb This is using the expo example, I tried it out on react-navigation and it works well 👍🏻 |
@jeremybarbet LOL, if add
to expo config, it works as expected I will add the new property |
Wow okay that's good to know! Don't you think After that, I'm good to merge. I'll probably release a |
No, because it's a bug in expo, for default android apps it should be false |
Okay, got it. I will find some time to merge and release. Thanks for the contributions! |
* fix: header component pan gesture handler for Android * feat: added new props and fixed keyboard handling * fix: options import for case sensetive fs * docs: fixed examples * docs: added new props * fix: examples * Revert "docs: fixed examples" This reverts commit 8696e78. * fix: watch:react-navigation script * fix: simplified handling of the modal height and keyboard handling * fix: keyboardAvoidingView enabled prop * fix: removed unnecessary component wrapper * feat: added the new avoidKeyboardLikeIOS prop * fix: updated expo examples
keyboardAvoidingOffset
,panGestureEnabled
, 'closeOnOverlayTap'#110
#109
#64
#59