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.

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

[createReactClass] Remove createReactClass from CameraRollView #21619

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
48 changes: 25 additions & 23 deletions Libraries/CameraRoll/CameraRoll.js
Original file line number Diff line number Diff line change
Expand Up @@ -79,34 +79,36 @@ const getPhotosParamChecker = createStrictShapeTypeChecker({
mimeTypes: PropTypes.arrayOf(PropTypes.string),
});

type GetPhotosReturn = Promise<{
edges: Array<{
node: {
type: string,
group_name: string,
image: {
uri: string,
height: number,
width: number,
isStored?: boolean,
playableDuration: number,
},
timestamp: number,
location?: {
latitude?: number,
longitude?: number,
altitude?: number,
heading?: number,
speed?: number,
},
export type GetPhotosEdge = {
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about renaming this to:

export type PhotoIdentifierObject = {

Or:

export type PhotoIdentifier = {

node: {
type: string,
group_name: string,
image: {
uri: string,
height: number,
width: number,
isStored?: boolean,
playableDuration: number,
},
}>,
timestamp: number,
location?: {
latitude?: number,
longitude?: number,
altitude?: number,
heading?: number,
speed?: number,
},
},
};

export type GetPhotosReturn = {
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about renaming this to:

export type PhotoIdentifierObjectsPage = {

Or:

export type PhotoIdentifiersPage = {

edges: Array<GetPhotosEdge>,
page_info: {
has_next_page: boolean,
start_cursor?: string,
end_cursor?: string,
},
}>;
};

/**
* Shape of the return value of the `getPhotos` function.
Expand Down Expand Up @@ -204,7 +206,7 @@ class CameraRoll {
*
* See https://facebook.github.io/react-native/docs/cameraroll.html#getphotos
*/
static getPhotos(params: GetPhotosParams): GetPhotosReturn {
static getPhotos(params: GetPhotosParams): Promise<GetPhotosReturn> {
if (__DEV__) {
checkPropTypes(
{params: getPhotosParamChecker},
Expand Down
8 changes: 6 additions & 2 deletions RNTester/js/CameraRollExample.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ const CameraRollView = require('./CameraRollView');

const AssetScaledImageExampleView = require('./AssetScaledImageExample');

import type {GetPhotosEdge} from 'CameraRoll';

class CameraRollExample extends React.Component<
$FlowFixMeProps,
$FlowFixMeState,
Expand Down Expand Up @@ -74,15 +76,17 @@ class CameraRollExample extends React.Component<
}
}

_renderImage = asset => {
_renderImage = (asset: GetPhotosEdge) => {
const imageSize = this.state.bigImages ? 150 : 75;
const imageStyle = [styles.image, {width: imageSize, height: imageSize}];
const {location} = asset.node;
const locationStr = location
? JSON.stringify(location)
: 'Unknown location';
return (
<TouchableOpacity key={asset} onPress={this.loadAsset.bind(this, asset)}>
<TouchableOpacity
key={asset.node.image.uri}
onPress={this.loadAsset.bind(this, asset)}>
<View style={styles.row}>
<Image source={asset.node.image} style={imageStyle} />
<View style={styles.info}>
Expand Down
186 changes: 94 additions & 92 deletions RNTester/js/CameraRollView.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,9 @@

'use strict';

var React = require('react');
var createReactClass = require('create-react-class');
const PropTypes = require('prop-types');
var ReactNative = require('react-native');
var {
const React = require('react');
const ReactNative = require('react-native');
const {
ActivityIndicator,
Alert,
CameraRoll,
Expand All @@ -25,105 +23,123 @@ var {
StyleSheet,
View,
} = ReactNative;
const ListViewDataSource = require('ListViewDataSource');

var groupByEveryN = require('groupByEveryN');
var logError = require('logError');
const groupByEveryN = require('groupByEveryN');
const logError = require('logError');

var propTypes = {
import type {GetPhotosEdge, GetPhotosReturn} from 'CameraRoll';

const rowHasChanged = function(r1: Array<Image>, r2: Array<Image>): boolean {
if (r1.length !== r2.length) {
return true;
}

for (var i = 0; i < r1.length; i++) {
if (r1[i] !== r2[i]) {
return true;
}
}

return false;
};

type Props = $ReadOnly<{|
/**
* The group where the photos will be fetched from. Possible
* values are 'Album', 'All', 'Event', 'Faces', 'Library', 'PhotoStream'
* and SavedPhotos.
*/
groupTypes: PropTypes.oneOf([
'Album',
'All',
'Event',
'Faces',
'Library',
'PhotoStream',
'SavedPhotos',
]),
groupTypes:
| 'Album'
| 'All'
| 'Event'
| 'Faces'
| 'Library'
| 'PhotoStream'
| 'SavedPhotos',

/**
* Number of images that will be fetched in one page.
*/
batchSize: PropTypes.number,
batchSize: number,

/**
* A function that takes a single image as a parameter and renders it.
*/
renderImage: PropTypes.func,
renderImage: GetPhotosEdge => React.Node,

/**
* imagesPerRow: Number of images to be shown in each row.
*/
imagesPerRow: PropTypes.number,

imagesPerRow: number,

/**
* The asset type, one of 'Photos', 'Videos' or 'All'
*/
assetType: PropTypes.oneOf(['Photos', 'Videos', 'All']),
};

var CameraRollView = createReactClass({
displayName: 'CameraRollView',
propTypes: propTypes,

getDefaultProps: function(): Object {
assetType: 'Photos' | 'Videos' | 'All',
|}>;

type State = {|
assets: Array<GetPhotosEdge>,
lastCursor: ?string,
noMore: boolean,
loadingMore: boolean,
dataSource: ListViewDataSource,
|};

class CameraRollView extends React.Component<Props, State> {
static defaultProps = {
groupTypes: 'SavedPhotos',
batchSize: 5,
imagesPerRow: 1,
assetType: 'Photos',
renderImage: function(asset: GetPhotosEdge) {
const imageSize = 150;
const imageStyle = [styles.image, {width: imageSize, height: imageSize}];
return <Image source={asset.node.image} style={imageStyle} />;
},
};

state = this.getInitialState();

getInitialState() {
return {
groupTypes: 'SavedPhotos',
batchSize: 5,
imagesPerRow: 1,
assetType: 'Photos',
renderImage: function(asset) {
var imageSize = 150;
var imageStyle = [styles.image, {width: imageSize, height: imageSize}];
return <Image source={asset.node.image} style={imageStyle} />;
},
};
},

getInitialState: function() {
var ds = new ListView.DataSource({rowHasChanged: this._rowHasChanged});

return {
assets: ([]: Array<Image>),
groupTypes: this.props.groupTypes,
lastCursor: (null: ?string),
assetType: this.props.assetType,
assets: [],
lastCursor: null,
noMore: false,
loadingMore: false,
dataSource: ds,
dataSource: new ListView.DataSource({rowHasChanged: rowHasChanged}),
};
},
}

/**
* This should be called when the image renderer is changed to tell the
* component to re-render its assets.
*/
rendererChanged: function() {
var ds = new ListView.DataSource({rowHasChanged: this._rowHasChanged});
rendererChanged() {
const ds = new ListView.DataSource({rowHasChanged: rowHasChanged});
this.state.dataSource = ds.cloneWithRows(
// $FlowFixMe(>=0.41.0)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the general rule of thumb should be: If you see a $FlowFixMe, try to see if you can remove it.

Pretty sure this is suppressing some valid flow errors. For example, rowHasChanged takes an array of Image components, but this.state.assets is of type Array<GetPhotosEdge>.

groupByEveryN(this.state.assets, this.props.imagesPerRow),
);
},
}

componentDidMount: function() {
componentDidMount() {
this.fetch();
},
}

/* $FlowFixMe(>=0.68.0 site=react_native_fb) This comment suppresses an error
* found when Flow v0.68 was deployed. To see the error delete this comment
* and run Flow. */
UNSAFE_componentWillReceiveProps: function(nextProps: {groupTypes?: string}) {
UNSAFE_componentWillReceiveProps(nextProps: {groupTypes?: string}) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove this $FlowFixMe?

Copy link
Contributor Author

@exced exced Oct 11, 2018

Choose a reason for hiding this comment

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

Not sure how we can remove it properly though.

This is the Flow error I get :
Cannot extend property Component [1] with CameraRollView because property groupTypes is read-only in object type [2] but writable in object type [3] in the first argument of property UNSAFE_componentWillReceiveProps.

if (this.props.groupTypes !== nextProps.groupTypes) {
this.fetch(true);
}
},
}

_fetch: async function(clear?: boolean) {
async _fetch(clear?: boolean) {
if (clear) {
this.setState(this.getInitialState(), this.fetch);
return;
Expand Down Expand Up @@ -162,21 +178,21 @@ var CameraRollView = createReactClass({
} catch (e) {
logError(e);
}
},
}

/**
* Fetches more images from the camera roll. If clear is set to true, it will
* set the component to its initial state and re-fetch the images.
*/
fetch: function(clear?: boolean) {
fetch = (clear?: boolean) => {
if (!this.state.loadingMore) {
this.setState({loadingMore: true}, () => {
this._fetch(clear);
});
}
},
};

render: function() {
render() {
return (
<ListView
renderRow={this._renderRow}
Expand All @@ -187,36 +203,22 @@ var CameraRollView = createReactClass({
enableEmptySections
/>
);
},
}

_rowHasChanged: function(r1: Array<Image>, r2: Array<Image>): boolean {
if (r1.length !== r2.length) {
return true;
}

for (var i = 0; i < r1.length; i++) {
if (r1[i] !== r2[i]) {
return true;
}
}

return false;
},

_renderFooterSpinner: function() {
_renderFooterSpinner = () => {
if (!this.state.noMore) {
return <ActivityIndicator />;
}
return null;
},
};

// rowData is an array of images
_renderRow: function(
rowData: Array<Image>,
_renderRow = (
rowData: Array<GetPhotosEdge>,
sectionID: string,
rowID: string,
) {
var images = rowData.map(image => {
) => {
const images = rowData.map(image => {
if (image === null) {
return null;
}
Expand All @@ -225,11 +227,11 @@ var CameraRollView = createReactClass({
});

return <View style={styles.row}>{images}</View>;
},
};

_appendAssets: function(data: Object) {
var assets = data.edges;
var newState: Object = {loadingMore: false};
_appendAssets(data: GetPhotosReturn) {
const assets = data.edges;
const newState: $Shape<State> = {loadingMore: false};

if (!data.page_info.has_next_page) {
newState.noMore = true;
Expand All @@ -245,16 +247,16 @@ var CameraRollView = createReactClass({
}

this.setState(newState);
},
}

_onEndReached: function() {
_onEndReached = () => {
if (!this.state.noMore) {
this.fetch();
}
},
});
};
}

var styles = StyleSheet.create({
const styles = StyleSheet.create({
row: {
flexDirection: 'row',
flex: 1,
Expand Down