Add ability to config onEndReachedThreshold and rowHasChanged props #76
Conversation
lughino
changed the title
components/ListView.js
Outdated
| @@ -67,8 +67,10 @@ class ListView extends React.Component { | |||
| static propTypes = { | |||
| autoHideHeader: React.PropTypes.bool, | |||
| style: React.PropTypes.object, | |||
| data: React.PropTypes.array, | |||
| data: React.PropTypes.oneOfType([ | |||
| React.PropTypes.object, React.PropTypes.array]), | |||
There was a problem hiding this comment.
When would you use object over array? Could you write an example of passed data as object and what would you expect to be rendered?
There was a problem hiding this comment.
Certainly, the component ListViewDataSource allows you to use the structure as an array or as an object (map). This means that you can pass a map structured in this way:
{
"postId1": {
"id": "postId1",
"title": "lorem ipsum",
....
},
"postId2": {
"id": "postId2",
"title": "lorem ipsum",
....
},
...
}Having the data in store with that structure, this means that you should not change them.
Actually it would also be given the option of passing the getRowData method (dataBlob, sectionID, rowID); which allows you to instruct the ListView to own structure.
components/ListView.js
Outdated
| @@ -67,8 +67,10 @@ class ListView extends React.Component { | |||
| static propTypes = { | |||
| autoHideHeader: React.PropTypes.bool, | |||
| style: React.PropTypes.object, | |||
| data: React.PropTypes.array, | |||
| data: React.PropTypes.oneOfType([ | |||
| React.PropTypes.object, React.PropTypes.array]), | |||
|
@MrBr could you take another look at it? |
package.json
Outdated
| "@shoutem/animation": "^0.9.0", | ||
| "@shoutem/theme": "^0.8.8", | ||
| "@shoutem/animation": "^0.8.10", | ||
| "@shoutem/theme": "git+https://github.com/Lughino/theme.git#0.9.0", |
There was a problem hiding this comment.
We can not depend on a fork for the theme module. Can you please make PR there as well.
After your PR is merged we can release new version.
What are the changes in the theme anyway?
| @@ -204,6 +216,7 @@ class DropDownMenu extends Component { | |||
| <ZoomOut driver={this.timingDriver} maxFactor={1.1} style={{ flex: 1 }}> | |||
| <FadeIn driver={this.timingDriver} style={{ flex: 1 }}> | |||
| <View style={style.modal} styleName="vertical"> | |||
| {header && <Title style={style.header}>{header}</Title>} | |||
There was a problem hiding this comment.
It would be nice if we had new method renderHeader and this expression there.
| static defaultProps = { | ||
| selectedIndex: 0, | ||
| showNextPage: false, | ||
| renderPlaceholder: () => { |
There was a problem hiding this comment.
One line would be nicer () => <LoadingIndicator/>.
| const { onIndexSelected } = this.props; | ||
| this.setState({ | ||
| selectedIndex: newIndex, | ||
| }, () => { |
There was a problem hiding this comment.
It would be nicer if we had selectedIndexUpdated method and if you would name argument selectedIndex then you could use object literals shorthand.
this.setState({ selectedIndex }, this.selectedIndexUpdated)
| import { connectStyle } from '@shoutem/theme'; | ||
| import { TimingDriver } from '@shoutem/animation'; | ||
|
|
||
| import Image from 'react-native-transformable-image'; |
There was a problem hiding this comment.
@zrumenjak & @SoHotSoup does it make sense maybe to include this in the toolkit? Maybe we could have prop on our image which would enable or disable such transformations (or change rendered component).
There was a problem hiding this comment.
I wouldn't include it in the toolkit for now. We are currently using this component only in some situations (iOS gallery, and lightbox), but we are also using a different, completely native, component in the gallery on Android.
| } | ||
| } | ||
|
|
||
| updateImageSwitchingStatus() { |
There was a problem hiding this comment.
Update prefix doesn't seems correct when we have no argument to which we update. There is certain logic in this function it does more then one thing so I would name itrefreshImageSwitchingStatus or resolveImageSwitchingStatus. That indicates that there is something going on.
You could easily have 3 methods from this one.
| ); | ||
| } | ||
|
|
||
| getImageTransformer(page) { |
There was a problem hiding this comment.
Can you keep render methods grouped together, it is much cleaner.
You can also group, update methods, get methods and other.
There was a problem hiding this comment.
You always pass here selectedIndex, better name then for the argument would be index.
| const description = _.get(page, 'description'); | ||
|
|
||
| if (!image) { | ||
| return; |
There was a problem hiding this comment.
Return null? How does this affect gallery? Should we maybe have fallback image?
| } | ||
| } | ||
|
|
||
| onSingleTapConfirmed() { |
There was a problem hiding this comment.
Can you document when will this be called.
|
|
||
| resetSurroundingImageTransformations() { | ||
| const { selectedIndex } = this.state; | ||
| let transformer = this.getImageTransformer(selectedIndex - 1); |
There was a problem hiding this comment.
This repeated code should be handled differently.
For example, you can implement getImageTransformers(indexes) and then
// Surrounding indexes
const indexes = [selectedIndex + 1, selectedIndex - 1];
const transformers = this.getImageTransformers(indexes);
transformers.forEach(this.resetImageTransformer);
Or you can have resetImageTransformer work with index and have condition in there (does transformer exists). But I like more the way it is now, it is clean.
No description provided.