-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Refactor picker to be a cross-platform component #2242
Changes from 11 commits
3ede3ce
54a0047
037dc18
b88ba8b
1c86bf7
e4bdc92
d571a87
ff00697
0a1823c
cd9618e
5a62d87
dcd92bc
6abfbb9
be475cd
d10d340
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
import React from 'react'; | ||
import RNPickerSelect from 'react-native-picker-select'; | ||
|
||
import * as pickerPropTypes from './pickerPropTypes'; | ||
import styles from '../../styles/styles'; | ||
|
||
const Picker = ({ | ||
onChange, | ||
items, | ||
useDisabledStyles, | ||
placeholder, | ||
value, | ||
icon, | ||
}) => ( | ||
<RNPickerSelect | ||
onValueChange={onChange} | ||
items={items} | ||
style={useDisabledStyles ? { | ||
...styles.picker, | ||
inputAndroid: [ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can be inputAndroid: [styles.picker.inputAndroid, styles.textInput, styles.disabledTextInput], |
||
styles.picker.inputAndroid, styles.textInput, styles.disabledTextInput, | ||
], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure how platform specific styles are handled but would it make more sense to have the ios/android/index files just export the style and the component itself be defined once? would avoid duplication and make it easier to update without having to touch 3 files |
||
} : styles.picker} | ||
useNativeAndroidPickerStyle={false} | ||
placeholder={placeholder} | ||
value={value} | ||
Icon={icon} | ||
/> | ||
); | ||
|
||
Picker.propTypes = pickerPropTypes.propTypes; | ||
Picker.defaultProps = pickerPropTypes.defaultProps; | ||
Picker.displayName = 'Picker'; | ||
|
||
export default Picker; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
import React from 'react'; | ||
import RNPickerSelect from 'react-native-picker-select'; | ||
|
||
import styles from '../../styles/styles'; | ||
import * as pickerPropTypes from './pickerPropTypes'; | ||
|
||
const Picker = ({ | ||
onChange, | ||
items, | ||
useDisabledStyles, | ||
placeholder, | ||
value, | ||
icon, | ||
}) => ( | ||
<RNPickerSelect | ||
onValueChange={onChange} | ||
items={items} | ||
style={useDisabledStyles ? { | ||
...styles.picker, | ||
inputIOS: [styles.picker.inputIOS, styles.textInput, styles.disabledTextInput], | ||
} : styles.picker} | ||
useNativeAndroidPickerStyle={false} | ||
placeholder={placeholder} | ||
value={value} | ||
Icon={icon} | ||
/> | ||
); | ||
|
||
Picker.propTypes = pickerPropTypes.propTypes; | ||
Picker.defaultProps = pickerPropTypes.defaultProps; | ||
Picker.displayName = 'Picker'; | ||
|
||
export default Picker; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
import React from 'react'; | ||
import RNPickerSelect from 'react-native-picker-select'; | ||
|
||
import styles from '../../styles/styles'; | ||
import * as pickerPropTypes from './pickerPropTypes'; | ||
|
||
const Picker = ({ | ||
onChange, | ||
items, | ||
useDisabledStyles, | ||
placeholder, | ||
value, | ||
icon, | ||
}) => ( | ||
<RNPickerSelect | ||
onValueChange={onChange} | ||
items={items} | ||
style={useDisabledStyles ? { | ||
...styles.picker, | ||
inputWeb: [styles.picker.inputWeb, styles.textInput, styles.disabledTextInput], | ||
} : styles.picker} | ||
useNativeAndroidPickerStyle={false} | ||
placeholder={placeholder} | ||
value={value} | ||
Icon={icon} | ||
/> | ||
); | ||
|
||
Picker.propTypes = pickerPropTypes.propTypes; | ||
Picker.defaultProps = pickerPropTypes.defaultProps; | ||
Picker.displayName = 'Picker'; | ||
|
||
export default Picker; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
import PropTypes from 'prop-types'; | ||
|
||
const propTypes = { | ||
// A callback method that is called when the value changes and it received the selected value as an argument | ||
onChange: PropTypes.func.isRequired, | ||
|
||
// Whether or not to show the disabled styles | ||
useDisabledStyles: PropTypes.bool, | ||
|
||
// The items to display in the list of selections | ||
items: PropTypes.arrayOf(PropTypes.shape({ | ||
// The value of the item that is being selected | ||
value: PropTypes.string.isRequired, | ||
|
||
// The text to display for the item | ||
label: PropTypes.string.isRequired, | ||
})).isRequired, | ||
|
||
// Something to show as the placeholder before something is selected | ||
placeholder: PropTypes.shape({ | ||
// The value of the placeholder item, usually an empty string | ||
value: PropTypes.string, | ||
|
||
// The text to be displayed as the placeholder | ||
label: PropTypes.string, | ||
}), | ||
|
||
// The value that needs to be selected | ||
value: PropTypes.oneOfType([PropTypes.number, PropTypes.string]), | ||
|
||
// An icon to display with the picker | ||
icon: PropTypes.func.isRequired, | ||
}; | ||
const defaultProps = { | ||
useDisabledStyles: false, | ||
placeholder: {}, | ||
value: null, | ||
}; | ||
|
||
export { | ||
propTypes, | ||
defaultProps, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,17 +4,17 @@ import ReportActionFragmentPropTypes from './ReportActionFragmentPropTypes'; | |
|
||
export default { | ||
// Name of the action e.g. ADDCOMMENT | ||
actionName: PropTypes.string.isRequired, | ||
actionName: PropTypes.string, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why the switch to making these prop optional? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a prop warning in the console that |
||
|
||
// Person who created the action | ||
person: PropTypes.arrayOf(ReportActionFragmentPropTypes).isRequired, | ||
person: PropTypes.arrayOf(ReportActionFragmentPropTypes), | ||
|
||
// ID of the report action | ||
sequenceNumber: PropTypes.number.isRequired, | ||
sequenceNumber: PropTypes.number, | ||
|
||
// Unix timestamp | ||
timestamp: PropTypes.number.isRequired, | ||
timestamp: PropTypes.number, | ||
|
||
// report action message | ||
message: PropTypes.arrayOf(ReportActionFragmentPropTypes).isRequired, | ||
message: PropTypes.arrayOf(ReportActionFragmentPropTypes), | ||
}; |
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'm not sure I understand why we need three different, largely identical files for the picker.
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.
is the style the only change? could we have that an input for another compnent perharphs? No real idea, just wandering
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.
Yeah, the styles are basically the only difference. I like the idea of just exporting the proper styles from the platform-specific files and DRYing this up more. I'll be working on that today.