Skip to content
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.

Sign up for GitHub

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

Jump to bottom

RequestorStep: Fix forward ref #28454

Merged
merged 9 commits into from
Nov 7, 2023
Merged

RequestorStep: Fix forward ref #28454

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
07172be
Fix refs
kacper-mikolajczak Sep 28, 2023
a8e58e5
add clarifying comment
kacper-mikolajczak Sep 29, 2023
3450b0f
Merge branch 'main' into fix/28390/requestor-step-refs
kacper-mikolajczak Oct 10, 2023
cda0b1f
forward ref in ScreenWrapper
kacper-mikolajczak Oct 10, 2023
a7bbb40
merge main
kacper-mikolajczak Oct 30, 2023
c1c3b46
Merge branch 'main' into fix/28390/requestor-step-refs
kacper-mikolajczak Nov 2, 2023
9adbed8
Merge branch 'main' into fix/28390/requestor-step-refs
kacper-mikolajczak Nov 6, 2023
4e8f849
fix android DatePicker propTypes
kacper-mikolajczak Nov 6, 2023
245a43a
Merge branch 'main' into fix/28390/requestor-step-refs
kacper-mikolajczak Nov 6, 2023
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
6 changes: 3 additions & 3 deletions src/components/DatePicker/index.android.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import styles from '@styles/styles';
import CONST from '@src/CONST';
import {defaultProps, propTypes} from './datepickerPropTypes';

function DatePicker({value, defaultValue, label, placeholder, errorText, containerStyles, disabled, onBlur, onInputChange, maxDate, minDate}, outerRef) {
const DatePicker = forwardRef(({value, defaultValue, label, placeholder, errorText, containerStyles, disabled, onBlur, onInputChange, maxDate, minDate}, outerRef) => {
const ref = useRef();

const [isPickerVisible, setIsPickerVisible] = useState(false);
Expand Down Expand Up @@ -70,10 +70,10 @@ function DatePicker({value, defaultValue, label, placeholder, errorText, contain
)}
</>
);
}
});

DatePicker.propTypes = propTypes;
DatePicker.defaultProps = defaultProps;
DatePicker.displayName = 'DatePicker';

export default forwardRef(DatePicker);
export default DatePicker;
274 changes: 140 additions & 134 deletions src/components/ScreenWrapper/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,149 +21,155 @@ import toggleTestToolsModal from '@userActions/TestTool';
import CONST from '@src/CONST';
import {defaultProps, propTypes} from './propTypes';

function ScreenWrapper({
shouldEnableMaxHeight,
shouldEnableMinHeight,
includePaddingTop,
keyboardAvoidingViewBehavior,
includeSafeAreaPaddingBottom,
shouldEnableKeyboardAvoidingView,
shouldEnablePickerAvoiding,
headerGapStyles,
children,
shouldShowOfflineIndicator,
offlineIndicatorStyle,
style,
shouldDismissKeyboardBeforeClose,
onEntryTransitionEnd,
testID,
}) {
const {windowHeight, isSmallScreenWidth} = useWindowDimensions();
const {initialHeight} = useInitialDimensions();
const keyboardState = useKeyboardState();
const {isDevelopment} = useEnvironment();
const {isOffline} = useNetwork();
const navigation = useNavigation();
const [didScreenTransitionEnd, setDidScreenTransitionEnd] = useState(false);
const maxHeight = shouldEnableMaxHeight ? windowHeight : undefined;
const minHeight = shouldEnableMinHeight ? initialHeight : undefined;
const isKeyboardShown = lodashGet(keyboardState, 'isKeyboardShown', false);

const isKeyboardShownRef = useRef();

isKeyboardShownRef.current = lodashGet(keyboardState, 'isKeyboardShown', false);

const panResponder = useRef(
PanResponder.create({
onStartShouldSetPanResponderCapture: (e, gestureState) => gestureState.numberActiveTouches === CONST.TEST_TOOL.NUMBER_OF_TAPS,
onPanResponderRelease: toggleTestToolsModal,
}),
).current;

const keyboardDissmissPanResponder = useRef(
PanResponder.create({
onMoveShouldSetPanResponderCapture: (e, gestureState) => {
const isHorizontalSwipe = Math.abs(gestureState.dx) > Math.abs(gestureState.dy);
const shouldDismissKeyboard = shouldDismissKeyboardBeforeClose && isKeyboardShown && Browser.isMobile();

return isHorizontalSwipe && shouldDismissKeyboard;
},
onPanResponderGrant: Keyboard.dismiss,
}),
).current;

useEffect(() => {
const unsubscribeTransitionEnd = navigation.addListener('transitionEnd', (event) => {
// Prevent firing the prop callback when user is exiting the page.
if (lodashGet(event, 'data.closing')) {
return;
}

setDidScreenTransitionEnd(true);
onEntryTransitionEnd();
});

// We need to have this prop to remove keyboard before going away from the screen, to avoid previous screen look weird for a brief moment,
// also we need to have generic control in future - to prevent closing keyboard for some rare cases in which beforeRemove has limitations
// described here https://reactnavigation.org/docs/preventing-going-back/#limitations
const beforeRemoveSubscription = shouldDismissKeyboardBeforeClose
? navigation.addListener('beforeRemove', () => {
if (!isKeyboardShownRef.current) {
return;
}
Keyboard.dismiss();
})
: undefined;

return () => {
unsubscribeTransitionEnd();

if (beforeRemoveSubscription) {
beforeRemoveSubscription();
}
};
// Rule disabled because this effect is only for component did mount & will component unmount lifecycle event
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);

return (
<SafeAreaConsumer>
{({insets, paddingTop, paddingBottom, safeAreaPaddingBottomStyle}) => {
const paddingStyle = {};

if (includePaddingTop) {
paddingStyle.paddingTop = paddingTop;
const ScreenWrapper = React.forwardRef(
(
{
shouldEnableMaxHeight,
shouldEnableMinHeight,
includePaddingTop,
keyboardAvoidingViewBehavior,
includeSafeAreaPaddingBottom,
shouldEnableKeyboardAvoidingView,
shouldEnablePickerAvoiding,
headerGapStyles,
children,
shouldShowOfflineIndicator,
offlineIndicatorStyle,
style,
shouldDismissKeyboardBeforeClose,
onEntryTransitionEnd,
testID,
},
ref,
) => {
const {windowHeight, isSmallScreenWidth} = useWindowDimensions();
const {initialHeight} = useInitialDimensions();
const keyboardState = useKeyboardState();
const {isDevelopment} = useEnvironment();
const {isOffline} = useNetwork();
const navigation = useNavigation();
const [didScreenTransitionEnd, setDidScreenTransitionEnd] = useState(false);
const maxHeight = shouldEnableMaxHeight ? windowHeight : undefined;
const minHeight = shouldEnableMinHeight ? initialHeight : undefined;
const isKeyboardShown = lodashGet(keyboardState, 'isKeyboardShown', false);

const isKeyboardShownRef = useRef();

isKeyboardShownRef.current = lodashGet(keyboardState, 'isKeyboardShown', false);

const panResponder = useRef(
PanResponder.create({
onStartShouldSetPanResponderCapture: (e, gestureState) => gestureState.numberActiveTouches === CONST.TEST_TOOL.NUMBER_OF_TAPS,
onPanResponderRelease: toggleTestToolsModal,
}),
).current;

const keyboardDissmissPanResponder = useRef(
PanResponder.create({
onMoveShouldSetPanResponderCapture: (e, gestureState) => {
const isHorizontalSwipe = Math.abs(gestureState.dx) > Math.abs(gestureState.dy);
const shouldDismissKeyboard = shouldDismissKeyboardBeforeClose && isKeyboardShown && Browser.isMobile();

return isHorizontalSwipe && shouldDismissKeyboard;
},
onPanResponderGrant: Keyboard.dismiss,
}),
).current;

useEffect(() => {
const unsubscribeTransitionEnd = navigation.addListener('transitionEnd', (event) => {
// Prevent firing the prop callback when user is exiting the page.
if (lodashGet(event, 'data.closing')) {
return;
}

// We always need the safe area padding bottom if we're showing the offline indicator since it is bottom-docked.
if (includeSafeAreaPaddingBottom || (isOffline && shouldShowOfflineIndicator)) {
paddingStyle.paddingBottom = paddingBottom;
setDidScreenTransitionEnd(true);
onEntryTransitionEnd();
});

// We need to have this prop to remove keyboard before going away from the screen, to avoid previous screen look weird for a brief moment,
// also we need to have generic control in future - to prevent closing keyboard for some rare cases in which beforeRemove has limitations
// described here https://reactnavigation.org/docs/preventing-going-back/#limitations
const beforeRemoveSubscription = shouldDismissKeyboardBeforeClose
? navigation.addListener('beforeRemove', () => {
if (!isKeyboardShownRef.current) {
return;
}
Keyboard.dismiss();
})
: undefined;

return () => {
unsubscribeTransitionEnd();

if (beforeRemoveSubscription) {
beforeRemoveSubscription();
}

return (
<View
style={[styles.flex1, {minHeight}]}
// eslint-disable-next-line react/jsx-props-no-spreading
{...(isDevelopment ? panResponder.panHandlers : {})}
testID={testID}
>
};
Comment on lines +90 to +108
Copy link
Contributor

Choose a reason for hiding this comment

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

This did not work as intended. It later caused #30268

// Rule disabled because this effect is only for component did mount & will component unmount lifecycle event
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);

return (
<SafeAreaConsumer>
{({insets, paddingTop, paddingBottom, safeAreaPaddingBottomStyle}) => {
const paddingStyle = {};

if (includePaddingTop) {
paddingStyle.paddingTop = paddingTop;
}

// We always need the safe area padding bottom if we're showing the offline indicator since it is bottom-docked.
if (includeSafeAreaPaddingBottom || (isOffline && shouldShowOfflineIndicator)) {
paddingStyle.paddingBottom = paddingBottom;
}

return (
<View
style={[styles.flex1, paddingStyle, ...style]}
ref={ref}
style={[styles.flex1, {minHeight}]}
// eslint-disable-next-line react/jsx-props-no-spreading
{...keyboardDissmissPanResponder.panHandlers}
{...(isDevelopment ? panResponder.panHandlers : {})}
testID={testID}
>
<KeyboardAvoidingView
style={[styles.w100, styles.h100, {maxHeight}]}
behavior={keyboardAvoidingViewBehavior}
enabled={shouldEnableKeyboardAvoidingView}
<View
style={[styles.flex1, paddingStyle, ...style]}
// eslint-disable-next-line react/jsx-props-no-spreading
{...keyboardDissmissPanResponder.panHandlers}
>
<PickerAvoidingView
style={styles.flex1}
enabled={shouldEnablePickerAvoiding}
<KeyboardAvoidingView
style={[styles.w100, styles.h100, {maxHeight}]}
behavior={keyboardAvoidingViewBehavior}
enabled={shouldEnableKeyboardAvoidingView}
>
<HeaderGap styles={headerGapStyles} />
{isDevelopment && <TestToolsModal />}
{isDevelopment && <CustomDevMenu />}
{
// If props.children is a function, call it to provide the insets to the children.
_.isFunction(children)
? children({
insets,
safeAreaPaddingBottomStyle,
didScreenTransitionEnd,
})
: children
}
{isSmallScreenWidth && shouldShowOfflineIndicator && <OfflineIndicator style={offlineIndicatorStyle} />}
</PickerAvoidingView>
</KeyboardAvoidingView>
<PickerAvoidingView
style={styles.flex1}
enabled={shouldEnablePickerAvoiding}
>
<HeaderGap styles={headerGapStyles} />
{isDevelopment && <TestToolsModal />}
{isDevelopment && <CustomDevMenu />}
{
// If props.children is a function, call it to provide the insets to the children.
_.isFunction(children)
? children({
insets,
safeAreaPaddingBottomStyle,
didScreenTransitionEnd,
})
: children
}
{isSmallScreenWidth && shouldShowOfflineIndicator && <OfflineIndicator style={offlineIndicatorStyle} />}
</PickerAvoidingView>
</KeyboardAvoidingView>
</View>
</View>
</View>
);
}}
</SafeAreaConsumer>
);
}
);
}}
</SafeAreaConsumer>
);
},
);

ScreenWrapper.displayName = 'ScreenWrapper';
ScreenWrapper.propTypes = propTypes;
Expand Down
12 changes: 9 additions & 3 deletions src/pages/ReimbursementAccount/RequestorStep.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,11 @@ const validate = (values) => {
return errors;
};

function RequestorStep({reimbursementAccount, shouldShowOnfido, onBackButtonPress, getDefaultStateForField}) {
/**
* Workaround for forwardRef + propTypes issue.
* See https://stackoverflow.com/questions/59716140/using-forwardref-with-proptypes-and-eslint
*/
const RequestorStep = React.forwardRef(({reimbursementAccount, shouldShowOnfido, onBackButtonPress, getDefaultStateForField}, ref) => {
const {translate} = useLocalize();

const defaultValues = useMemo(
Expand Down Expand Up @@ -108,6 +112,7 @@ function RequestorStep({reimbursementAccount, shouldShowOnfido, onBackButtonPres
if (shouldShowOnfido) {
return (
<RequestorOnfidoStep
ref={ref}
reimbursementAccount={reimbursementAccount}
onBackButtonPress={onBackButtonPress}
/>
Expand All @@ -116,6 +121,7 @@ function RequestorStep({reimbursementAccount, shouldShowOnfido, onBackButtonPres

return (
<ScreenWrapper
ref={ref}
includeSafeAreaPaddingBottom={false}
testID={RequestorStep.displayName}
>
Expand Down Expand Up @@ -190,9 +196,9 @@ function RequestorStep({reimbursementAccount, shouldShowOnfido, onBackButtonPres
</Form>
</ScreenWrapper>
);
}
});

RequestorStep.propTypes = propTypes;
RequestorStep.displayName = 'RequestorStep';

export default React.forwardRef(RequestorStep);
export default RequestorStep;
Loading