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 (from #629) #705

Merged
merged 52 commits into from May 30, 2018

Conversation

@xcarpentier
Collaborator

xcarpentier commented Jan 15, 2018

From #629

@xcarpentier xcarpentier changed the title from Modified for flatlist to Modified for flatlist (from #629) Jan 15, 2018

@xcarpentier xcarpentier referenced this pull request Jan 15, 2018

Closed

Modified for FlatList #629

@codecov

This comment has been minimized.

codecov bot commented Jan 15, 2018

Codecov Report

Merging #705 into master will decrease coverage by 0.69%.
The diff coverage is 23.33%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #705     +/-   ##
=========================================
- Coverage   43.17%   42.47%   -0.7%     
=========================================
  Files          20       20             
  Lines         498      485     -13     
  Branches      104      106      +2     
=========================================
- Hits          215      206      -9     
+ Misses        216      210      -6     
- Partials       67       69      +2
Impacted Files Coverage Δ
src/Bubble.js 46.15% <ø> (ø) ⬆️
src/Send.js 71.42% <ø> (ø) ⬆️
src/Composer.js 30.76% <ø> (ø) ⬆️
src/Day.js 85.71% <ø> (ø) ⬆️
src/InputToolbar.js 63.33% <0%> (-4.53%) ⬇️
src/Avatar.js 81.25% <100%> (ø) ⬆️
src/utils.js 87.5% <100%> (+45.83%) ⬆️
src/Message.js 60.71% <100%> (ø) ⬆️
src/MessageContainer.js 25.64% <14.28%> (-13.25%) ⬇️
src/MessageText.js 43.33% <50%> (-1.5%) ⬇️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cbbf812...b55d841. Read the comment docs.

@xcarpentier xcarpentier requested a review from cooperka Jan 16, 2018

@xcarpentier

This comment has been minimized.

Collaborator

xcarpentier commented Jan 16, 2018

I think we need to have an expo publish on every PR to test easely...

@cooperka

This comment has been minimized.

Collaborator

cooperka commented Jan 16, 2018

Thanks for cleaning this up @xcarpentier 🍻 But fyi I probably won't have time to review this week, you may want to find someone else to review.

@cooperka cooperka removed their request for review Jan 16, 2018

@xcarpentier xcarpentier requested a review from brunocascio Jan 16, 2018

@brunocascio

This comment has been minimized.

Collaborator

brunocascio commented Jan 16, 2018

Great work @xcarpentier !

I am in vacation now, but I saw part of the code. Is there a way to remove internal state and use only props in order to make this lib completely stateless? What do you think?

@xcarpentier xcarpentier added this to In progress in react-native-gifted-chat Jan 17, 2018

@xcarpentier xcarpentier self-assigned this Jan 17, 2018

{...invertibleScrollViewProps}
ref={(component) => (this._invertibleScrollViewRef = component)}
/>
<View style={{ transform: [{ scaleY: -1 }, { perspective: 1280 }] }}>

This comment has been minimized.

@brunolemos
enableEmptySections
automaticallyAdjustContentInsets={false}
initialListSize={20}
pageSize={20}
ref="flatListRef"

This comment has been minimized.

@brunolemos

brunolemos Jan 17, 2018

String ref was deprecated a year ago, you should use callback refs only.

ref={flatListRef =>
  if (flatListRef) {
    this.flatListRef = flatListRef
  }
}
renderItem={this.renderRow}
renderHeader={this.renderFooter}
renderFooter={this.renderLoadEarlier()}
style={{ transform: [{ scaleY: -1 }, { perspective: 1280 }] }}

This comment has been minimized.

@brunolemos
justifyContent: 'flex-end',
headerWrapper: {
flex: 1,
transform: [{ scaleY: -1 }, { perspective: 1280 }],

This comment has been minimized.

@brunolemos
pageSize={20}
renderFooter={[Function]}
renderFooter={null}

This comment has been minimized.

@brunolemos
"scaleY": -1,
},
Object {
"perspective": 1280,

This comment has been minimized.

@brunolemos
"scaleY": -1,
},
Object {
"perspective": 1280,

This comment has been minimized.

@brunolemos
@brunolemos

I found very weird that the code transform: [{ scaleY: -1 }, { perspective: 1280 }] appears at least 4 times in this PR and I'm not sure why they are necessary. Maybe this can be optimized? Implemented differently?

xcarpentier added some commits Jan 23, 2018

@xcarpentier

This comment has been minimized.

Collaborator

xcarpentier commented Jan 24, 2018

Thanks, @brunolemos I removed this style transform: [{ scaleY: -1 }, { perspective: 1280 }]

@xcarpentier

This comment has been minimized.

Collaborator

xcarpentier commented Jan 24, 2018

Thanks @brunocascio, I removed state and extends PureComponent

@brunolemos

brunolemos suggested changes May 15, 2018 edited

We found a bug that only happens when using this branch.

We have something like <GiftedChat loadEarlier={this.props.loadEarlier} ... />, that works just fine on current live version, but on this branch it doesn't show the Load Earlier button on the initial load, only after I send a message.

The fix is to set extraData in the FlatList, otherwise it won't trigger a re-render.


  1. This commit fixes it using getDerivedStateFromProps. Requires React 16.3.
  2. Doing extraData={this.props} should also fix it, but an overkill, because it would trigger more re-renders than necessary
  3. You'll probably need to think of a third alternative, and also take in consideration other props besides loadEarlier, like renderFooter.
@brunolemos

brunolemos suggested changes May 15, 2018 edited

Here is another issue that only happens on this branch.

If there are no messages and you tap the TextInput, the TextInput won't move up, it will stay below the keyboard.

If there are messages, it works as expected.

The third image shows the issue. The second image if from the live version.

Does anyone know what caused this one?

contentContainerStyle={styles.contentContainerStyle}
renderItem={this.renderRow}
renderHeader={this.renderFooter}
renderFooter={this.renderLoadEarlier}

This comment has been minimized.

@brunolemos

brunolemos May 15, 2018

Why is renderHeader=renderFooter and renderFooter=renderLoadEarlier? 🤪

@howg0924

This comment has been minimized.

howg0924 commented May 15, 2018

@brunolemos as for the TextInput does not move up problem, I fixed it in my fork, maybe you could take it as a reference: howg0924@06e78be

I solved it by calling keyboard listeners directly in GiftedChat.js. I don't understand why these listeners were passed down to the FlatList in MessageContainer.js.

The problem only happens on iOS on my app.

@brunolemos

This comment has been minimized.

brunolemos commented May 15, 2018

@howg0924 unfortunately cherry-picking this commit did not solve the issue... maybe because your branch is outdated?

@howg0924

This comment has been minimized.

howg0924 commented May 15, 2018

@brunolemos: My fork is not outdated on this branch. I also fixed some other issues and did a little enhancement on my fork. I guess maybe these modification also helped to fixed the issue. Or maybe the root cause of your problem is different from mine.

@brunolemos

This comment has been minimized.

brunolemos commented May 15, 2018

@howg0924 yeah I merged master on it, maybe it was that

@b4stien

This comment has been minimized.

b4stien commented May 21, 2018

Hello everyone, I am not sure what's blocking this PR, can we help in any way?

We're using this project at my company and would be willing to help land this (which seems to be fixing a rendering issue with messages not appearing until scroll).

@xcarpentier

This comment has been minimized.

Collaborator

xcarpentier commented May 21, 2018

@b4stien some performance issue. Need to add some suc and maybe change some pure component

@sibelius

This comment has been minimized.

sibelius commented May 29, 2018

@FaridSafi can we merge this, and track some regression in new issues?

@xcarpentier

This comment has been minimized.

Collaborator

xcarpentier commented May 29, 2018

@sibelius I will merge it tomorrow.

@helielson

This comment has been minimized.

Contributor

helielson commented May 29, 2018

Nice @xcarpentier! Can't wait! Let me know if you need any help

@xcarpentier xcarpentier merged commit c143b5f into master May 30, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
ci/circleci Your tests passed on CircleCI!
Details

react-native-gifted-chat automation moved this from In progress to Done May 30, 2018

@jaburcodes

This comment has been minimized.

jaburcodes commented May 30, 2018

awesome PR!! Thanks @xcarpentier pretty sure this will help a lot of devs!

@dead23angel

This comment has been minimized.

dead23angel commented May 30, 2018

bump version pls

this.setState({
position: 'relative',
});
if (this.state !== 'relative') {

This comment has been minimized.

@varungupta85

varungupta85 May 30, 2018

Should this be this.state.position? Same for the if condition in keyboardWillHide also.

This comment has been minimized.

@brunolemos

brunolemos May 30, 2018

This is probably what caused this bug: #705 (review)

@fbartho

This comment has been minimized.

Contributor

fbartho commented May 30, 2018

@varungupta85 that bug looks like it just got merged into master!

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
@ChrisEdson

This comment has been minimized.

ChrisEdson commented Jun 18, 2018

Is it possible to get a release with the new flat list design?

@glebez

This comment has been minimized.

Contributor

glebez commented Jun 28, 2018

Is it just me, or rednerFooter prop of gifted chat stopped properly working after this branch has been merged?

On first sight it seems to me, that FlatList does not expect props renderHeader and renderFooter, but instead ListHeaderComponent and ListFooterComponent, which makes them be ignored.

@xcarpentier

This comment has been minimized.

Collaborator

xcarpentier commented Jun 28, 2018

@glebez Do you have any screenshot or code to share?
Or maybe think about create a PR with the fix...

@sibelius

This comment has been minimized.

sibelius commented Jun 28, 2018

We should have an issue with all possible regressions of this PR

@glebez

This comment has been minimized.

Contributor

glebez commented Jun 28, 2018

@xcarpentier, I'll try to put together a reduced test case first and afterwards a PR.

@glebez

This comment has been minimized.

Contributor

glebez commented Jun 28, 2018

Here's a test case using current npm package, so before the FlatList was introduced and the footer is displayed properly: https://snack.expo.io/@glebkost/listview-example

If you change the package to use current master the footer is not displayed. (Unfortunately it's not possible in expo snack to set the package to github url, so you need to copy the snack to your local env to test that).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment