-
Notifications
You must be signed in to change notification settings - Fork 589
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
browser: android fixes v2 #5712
Conversation
@@ -529,7 +538,6 @@ const styles = StyleSheet.create({ | |||
overflow: 'hidden', | |||
top: 0, | |||
width: DEVICE_WIDTH, | |||
zIndex: 50000, |
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.
over 9000
}, [canGoBack, canGoForward, isFavorite, tabUrl]); | ||
|
||
const onPressMenuItem = useCallback( | ||
async ({ nativeEvent: { actionKey } }: { nativeEvent: { actionKey: 'share' | 'favorite' | 'back' | 'forward' } }) => { |
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.
would prefer these all to be typed properly going forward.
import { OnPressMenuItemEventObject } from 'react-native-ios-context-menu';
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.
small nits but code looks good.
options: menuConfig.menuItems.map(item => item?.actionTitle), | ||
}, | ||
(buttonIndex: number) => { | ||
onPressMenuItem({ nativeEvent: { actionKey: menuConfig.menuItems[buttonIndex]?.actionKey as any } }); |
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.
same here, use the proper type:
import { OnPressMenuItemEventObject } from 'react-native-ios-context-menu';
} else { | ||
const url = activeTabInfo.value.url; | ||
if (url) handleShareUrl(url); |
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 this clause will be hit on android if they cancel the action sheet.
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.
only change that's needed. Others are nits @skylarbarrera
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.
im gonna merge this in and handle in my next PR, nice catch
Fixes APP-1342
Fixes APP-1436
Fixes APP-1340
also added i18n where missing, fixed some small android style issues
What changed (plus any additional context for devs)
Screen recordings / screenshots
What to test