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

Use defaultProps instead of hardcoded value #10567

Merged
merged 5 commits into from
Aug 25, 2022
Merged

Use defaultProps instead of hardcoded value #10567

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
56149b3
Use defaultProps instead of hardcoded value
roryabraham Aug 25, 2022
34ca065
Add empty string as option
roryabraham Aug 25, 2022
1e27aca
Refactor
roryabraham Aug 25, 2022
b83498b
Remove bad line
roryabraham Aug 25, 2022
b0807d5
Fix lint
roryabraham Aug 25, 2022
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
3 changes: 3 additions & 0 deletions src/components/ScreenWrapper/BaseScreenWrapper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import {KeyboardAvoidingView, View} from 'react-native';
import React from 'react';
import PropTypes from 'prop-types';
import {SafeAreaInsetsContext} from 'react-native-safe-area-context';
import _ from 'underscore';
import {withOnyx} from 'react-native-onyx';
Expand All @@ -19,6 +20,8 @@ import ONYXKEYS from '../../ONYXKEYS';
import {withNetwork} from '../OnyxProvider';
import {propTypes, defaultProps} from './propTypes';

propTypes.keyboardAvoidingViewBehavior = PropTypes.oneOf(['padding', 'height', 'position', '']).isRequired;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not add these to propTypes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are in the prop types, but they're not required. index.android.js and index.js provide defaults, but BaseScreenWrapper does not, so I just added it as a required prop here

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry why does it need to be required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see what you were suggesting. Done!

Copy link
Contributor

Choose a reason for hiding this comment

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

we are still passing a default in the defaultProps for this. so I think we can just remove this and if we want to be able to pass a string then add it here it's ok

App/src/components/ScreenWrapper/propTypes.js

Lines 24 to 25 in e6b6dd2

keyboardAvoidingViewBehavior: PropTypes.oneOf(['padding', 'height', 'position']),

?

Copy link
Contributor

Choose a reason for hiding this comment

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

Think we still wanted to add the '' here but maybe it's fine it will just do a warning 🤷


class BaseScreenWrapper extends React.Component {
constructor(props) {
super(props);
Expand Down
3 changes: 2 additions & 1 deletion src/components/ScreenWrapper/index.android.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,12 @@ import React from 'react';
import BaseScreenWrapper from './BaseScreenWrapper';
import {defaultProps, propTypes} from './propTypes';

defaultProps.keyboardAvoidingViewBehavior = 'height';
roryabraham marked this conversation as resolved.
Show resolved Hide resolved

const ScreenWrapper = props => (
<BaseScreenWrapper
// eslint-disable-next-line react/jsx-props-no-spreading
{...props}
keyboardAvoidingViewBehavior="height"
>
{props.children}
</BaseScreenWrapper>
Expand Down
2 changes: 2 additions & 0 deletions src/components/ScreenWrapper/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ import React from 'react';
import BaseScreenWrapper from './BaseScreenWrapper';
import {defaultProps, propTypes} from './propTypes';

defaultProps.keyboardAvoidingViewBehavior = 'padding';
roryabraham marked this conversation as resolved.
Show resolved Hide resolved

const ScreenWrapper = props => (
<BaseScreenWrapper
// eslint-disable-next-line react/jsx-props-no-spreading
Expand Down
1 change: 0 additions & 1 deletion src/components/ScreenWrapper/propTypes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ const defaultProps = {
includePaddingTop: true,
onTransitionEnd: () => {},
modal: {},
keyboardAvoidingViewBehavior: 'padding',
roryabraham marked this conversation as resolved.
Show resolved Hide resolved
};

export {propTypes, defaultProps};
7 changes: 5 additions & 2 deletions src/pages/home/ReportScreen.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import React from 'react';
import {withOnyx} from 'react-native-onyx';
import PropTypes from 'prop-types';
import {Keyboard, View} from 'react-native';
import {Keyboard, Platform, View} from 'react-native';
import lodashGet from 'lodash/get';
import _ from 'underscore';
import lodashFindLast from 'lodash/findLast';
Expand Down Expand Up @@ -227,7 +227,10 @@ class ReportScreen extends React.Component {
reportClosedAction = lodashFindLast(this.props.reportActions, action => action.actionName === CONST.REPORT.ACTIONS.TYPE.CLOSED);
}
return (
<ScreenWrapper style={[styles.appContent, styles.flex1, {marginTop: this.state.viewportOffsetTop}]}>
<ScreenWrapper
style={[styles.appContent, styles.flex1, {marginTop: this.state.viewportOffsetTop}]}
keyboardAvoidingViewBehavior={Platform.OS === 'android' ? '' : 'padding'}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We would refactor this to follow our JS guidelines

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or maybe we just ... wont? IDK...

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean if we want we could create something like getReportScreenScreenWrapperStyles() -> index.js 'padding' index.android.js '', but not really worth IMO.

Copy link
Contributor

Choose a reason for hiding this comment

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

Coming in here because I ran into this warning:

image

Question: Why doesn't this default to "height" for android since that's what's being done in the android file above?

Request: Please refactor this to follow our platform conventions. Leaving code like this only let's contributors see the pattern and follow it. I recently read through the README section "Cross Platform 99.9999%" and it's a good refresher of why we do this. We should not compromise here.

Copy link
Contributor

Choose a reason for hiding this comment

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

There was some conversation about this being temporary to help unblock the deploy but I'm guessing it just did not get cleaned up.

Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC @Julesssss found a better solution for this problem.

roryabraham marked this conversation as resolved.
Show resolved Hide resolved
>
<HeaderView
reportID={reportID}
onNavigationMenuButtonClicked={() => Navigation.navigate(ROUTES.HOME)}
Expand Down