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

[createReactClass] Remove createReactClass from CameraRollView #21619

Closed
wants to merge 6 commits into from
Closed

[createReactClass] Remove createReactClass from CameraRollView #21619

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
275858c
UPDATE: rm createReactClass
exced Oct 10, 2018
bad2a02
Merge branch 'master' of https://github.com/facebook/react-native int…
exced Oct 10, 2018
c84b563
UPDATE: flow types
exced Oct 10, 2018
54ca1b9
Merge branch 'master' of https://github.com/facebook/react-native int…
exced Oct 10, 2018
18dcaa1
Merge branch 'master' of https://github.com/facebook/react-native int…
exced Oct 11, 2018
77acac9
UPDATE: rm $FlowFixMe
exced Oct 11, 2018
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
164 changes: 82 additions & 82 deletions RNTester/js/CameraRollView.js
View file
Open in desktop
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,111 @@ var {
StyleSheet,
View,
} = ReactNative;
const ListViewDataSource = require('ListViewDataSource');

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

import type {RNTesterProps} from 'RNTesterTypes';

type Props = $ReadOnly<{|
...RNTesterProps,
Copy link
Contributor

Choose a reason for hiding this comment

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

How come you're spreading the RNTesterProps into Props?


var propTypes = {
/**
* 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: $FlowFixMe => React.Node,
Copy link
Contributor

Choose a reason for hiding this comment

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

this._fetch calls CameraRoll.getPhotos and then calls this._appendAssets after waiting for the CameraRoll.getPhotos promise to resolve. this._appendAssets groups the result into rows, and passes the rows to the data source this._ds. This eventually calls this._renderRow, which calls this._renderImage. So, we can actually figure out what this type is, and remove this $FlowFixMe.

CameraRoll.getPhotos contains the type information. (I think we should export it, if necessary).


/**
* 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<Image>,
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: $FlowFixMe) {
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: this._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: this._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 •
edited
Loading

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) {
_fetch = async (clear?: boolean) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't need to be bound:

  async _fetch(clear?: boolean) {

if (clear) {
this.setState(this.getInitialState(), this.fetch);
return;
Expand Down Expand Up @@ -162,21 +166,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,9 +191,9 @@ var CameraRollView = createReactClass({
enableEmptySections
/>
);
},
}

_rowHasChanged: function(r1: Array<Image>, r2: Array<Image>): boolean {
_rowHasChanged(r1: Array<Image>, r2: Array<Image>): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be image. 😞

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, this doesn't have to be a method on the component class.

if (r1.length !== r2.length) {
return true;
}
Expand All @@ -201,22 +205,18 @@ var CameraRollView = createReactClass({
}

return false;
},
}

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

// rowData is an array of images
_renderRow: function(
rowData: Array<Image>,
sectionID: string,
rowID: string,
) {
var images = rowData.map(image => {
_renderRow = (rowData: Array<Image>, sectionID: string, rowID: string) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

rowData isn't an array of Image components.

const images = rowData.map(image => {
if (image === null) {
return null;
}
Expand All @@ -225,11 +225,11 @@ var CameraRollView = createReactClass({
});
exced marked this conversation as resolved.
Show resolved Hide resolved

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

_appendAssets: function(data: Object) {
var assets = data.edges;
var newState: Object = {loadingMore: false};
_appendAssets = (data: Object) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be bound.

const assets = data.edges;
const newState: Object = {loadingMore: false};
Copy link
Contributor

Choose a reason for hiding this comment

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

const newState: $Shape<State> = { loadingMore: false };


if (!data.page_info.has_next_page) {
newState.noMore = true;
Expand All @@ -245,16 +245,16 @@ var CameraRollView = createReactClass({
}
exced marked this conversation as resolved.
Show resolved Hide resolved

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