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

Modified for FlatList #629

Closed
wants to merge 84 commits into
base: master
from

Conversation

Projects
None yet
@gavin-gmlab
Copy link

gavin-gmlab commented Nov 6, 2017

Implementation for FlatList instead of ListView component

@radko93
Copy link

radko93 left a comment

Please remove react-native-invertible-scroll-view from dependencies.

this.setState({
dataSource: this.state.dataSource.cloneWithRows(messagesData.blob, messagesData.keys)
});
messagesData: this.prepareMessages(nextProps.messages)

This comment has been minimized.

@sondremare

sondremare Nov 6, 2017

This will cause all chat messages to re-render every time a new message is added.

This comment has been minimized.

@redwind

redwind Nov 8, 2017

i think we should remove prepareMessages and move set value for nextMessage and previousMessage into renderItem function

This comment has been minimized.

@gavin-gmlab

gavin-gmlab Nov 21, 2017

Author

My thoughts:
Its not worse than what it was with the ListView approach. Correct me if I am wrong since the old approach was rerendering the whole ListView anyway. FlatList is supposed to render only what is actually viewable anyway, and not those components that are outside the view, so we are getting an improvement right there.

I was more thinking of getting a quick fix in for now, and we can always have perf improvements later? But just my opinion.

This comment has been minimized.

@dralletje

dralletje Nov 26, 2017

It is not the prepareMessages that will cause everything (in the view) to rerender, that will happen anyway when a new message arrives. What we can do about that is make the components being rendered pure, and allow them to be effective PureComponent-s

@wizza-smile

This comment has been minimized.

Copy link

wizza-smile commented Nov 26, 2017

maybe you guys have some thoughts to share on this! @Kabangi @radko93 @dralletje @bartolkaruza @DavidKuennen @Ashoat

return output = messages.reduce((o,m,i) => {
const previousMessage = messages[i + 1] || {}
const nextMessage = messages[i - 1] || {}
o.push( {

This comment has been minimized.

@dralletje

dralletje Nov 26, 2017

Two small questions here:

  1. It seems to me this is a lot simpler done using .map, maybe even more performant? (Dunno if that even matters here)

  2. Because you merge previousMessage and nextMessage props onto the existing message, that message will be "invalidated" when matching it against existing messages (causing PureComponent or shouldComponentUpdate to always see change). I'd recommend setting { currentMessage, previousMessage, nextMessage } and properly using item.currentMessage instead of just item in https://github.com/FaridSafi/react-native-gifted-chat/pull/629/files#diff-bd8395a0a94eca8b61a34ec66f8a605cR102

It might be that I miss something why this is necessary, let me know :)

This comment has been minimized.

@dralletje

dralletje Nov 26, 2017

About the second one, I see that Message isn't PureComponent yet, but I think it is still good to these optimisations already, if we want Message to be an effective PureComponent later ;)

https://github.com/gavin-gmlab/react-native-gifted-chat/blob/33dc31f41d29be98e441c4b27ad74e453b9801b5/src/Message.js#L16

currentMessage: item,
previousMessage: item.previousMessage,
nextMessage: item.nextMessage,
position: item.user._id === this.props.user._id ? 'right' : 'left',
};

if (this.props.renderMessage) {
return this.props.renderMessage(messageProps);

This comment has been minimized.

@dralletje

dralletje Nov 26, 2017

Is it on purpose that this is outside of the { transform: [{ scaleY: -1 }] } ? I don't think people will want to have to set that themselves, and when we go to use invert={true} on the container later, the people that do have their own { transform: [{ scaleY: -1 }] } will have to remove it again and ughhhhh hahaha :)

renderFooter={this.renderLoadEarlier}
renderScrollComponent={this.renderScrollComponent}
renderFooter={this.renderLoadEarlier()}
style={{transform: [{scaleY: -1 },{perspective: 1280}]}}

This comment has been minimized.

@dralletje

dralletje Nov 26, 2017

One last thing, what is the reasoning behind using this transform ourselves, and not using inverted={true}? I'm not sure why it not in the documentation (so you may very well know something I don't) :-)

But this would solve the problem of us having to set the transform on all the items, and it would also spare us a view per item (FlatList applies inverted styles on the view it would create anyway, we introduce a new one :-/)

I understand if it is for version support: I think inverted only is supported in a couple of last version

https://github.com/facebook/react-native/blob/master/Libraries/Lists/FlatList.js#L142

This comment has been minimized.

@sondremare

sondremare Nov 26, 2017

Inverted works only on RN 0.49 and above. Atleast for Huawei phones running Android 7

This comment has been minimized.

@dralletje

dralletje Nov 26, 2017

Then maybe it is even a combination of the fact that inverted got added later + the perspective fix... Maybe good to wait some version before we use inverted, but still worth adding a TODO/NOTE?

apparition47 and others added some commits Dec 7, 2017

@xcarpentier

This comment has been minimized.

Copy link
Collaborator

xcarpentier commented on src/GiftedChat.js in 7157e8e Dec 15, 2017

Why not using configurable props for that?

This comment has been minimized.

Copy link
Collaborator

xcarpentier replied Dec 15, 2017

You merged the babd PR, #658
Can you confirm @apparition47 ?

This comment has been minimized.

Copy link
Contributor

apparition47 replied Dec 15, 2017

Yes, he merged the other PR. My PR is still open.

EDIT: merged already

xcarpentier and others added some commits Dec 15, 2017

Merge pull request #658 from apparition47/inverted-prop
Inverted prop for reverse messages display order
@ngothanhtai

This comment has been minimized.

Copy link

ngothanhtai commented on src/GiftedChat.js in 528aa0e Dec 18, 2017

@xcarpentier: how can we use this inside a static function?

This comment has been minimized.

Copy link
Collaborator Author

xcarpentier replied Dec 18, 2017

Yes you are right... I’ve not seen that!

But this commit is just a fix of getting inverted from props...

this.inverted was already there before.

We need to figures out how to fix it... do you have any suggestion?

This comment has been minimized.

Copy link
Collaborator Author

xcarpentier replied Dec 18, 2017

@ngothanhtai I changed it by passing it to params with true as default value

This comment has been minimized.

Copy link

ngothanhtai replied Dec 18, 2017

That's OK to add parameter with default value. Thanks @xcarpentier :)

greenkeeper[bot] and others added some commits Jan 11, 2018

@xcarpentier

This comment has been minimized.

Copy link
Collaborator

xcarpentier commented Jan 15, 2018

I've been creating #705 to have a clean history.

Please let me know if I can close this one?

@gavin-gmlab

This comment has been minimized.

Copy link
Author

gavin-gmlab commented Jan 17, 2018

@xcarpentier , yes, feel free to close this PR

@brunocascio

This comment has been minimized.

Copy link
Collaborator

brunocascio commented on src/Actions.js in 4571dda Mar 15, 2018

@xcarpentier
After change to non-arrow function, this code brokes because this.props is undefined (since this is the context of launc instead of handle.

I'm going to fix now.

@pentarex

This comment has been minimized.

Copy link
Contributor

pentarex commented on src/Time.js in 5b67203 Mar 16, 2018

is this a mistake or am I missing something? Why the textStyle is not a property anymore?

This comment has been minimized.

Copy link
Collaborator Author

xcarpentier replied Mar 17, 2018

Good catch, can you make a PR?
Sorry 🙄

This comment has been minimized.

Copy link
Contributor

pentarex replied Mar 17, 2018

#797 is the issue #798 is the PR :)

xcarpentier added a commit that referenced this pull request May 30, 2018

Modified for flatlist (from #629) (#705)
Modified for FlatList

chilinh added a commit to chilinh/react-native-gifted-chat that referenced this pull request Jun 18, 2018

Merge branch 'r_master'
* r_master:
  Move listViewProps to the end (FaridSafi#882)
  chore(package): update react-test-renderer to version 16.4.0 (FaridSafi#868)
  chore(package): update babel-jest to version 23.0.0 (FaridSafi#869)
  Update and Fix Slack Example (FaridSafi#895)
  Remove prop removeClippedSubviews on Flatlist (FaridSafi#897)
  Update Chatkit readme.md
  Fix typos in TS Types (FaridSafi#877)
  Modified for flatlist (from FaridSafi#629) (FaridSafi#705)
  fix tabs
  ignore example folder
  Add Chatkit asset to README.md
  Add Chatkit banner asset
  chore(package): update react-test-renderer to version 16.3.2 (FaridSafi#829)
  Update README.md (FaridSafi#827)
  add image beacon to readme
  chore(package): update react-test-renderer to version 16.3.1 (FaridSafi#821)

# Conflicts:
#	.eslintignore
#	src/MessageContainer.js

hammadj added a commit to hammadj/react-native-gifted-chat that referenced this pull request Dec 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.