Skip to content

Conversation

@vishalnarkhede
Copy link
Contributor

@vishalnarkhede vishalnarkhede commented Jul 27, 2020

Submit a pull request

CLA

  • I have signed the Stream CLA (required).
  • The code changes follow best practices
  • Code changes are tested (add some information if not applicable)

Description of the pull request

  • Config setup for jest and native-testing-library
  • Adding tests for ChannelPreview and ChannelPreviewMessenger
  • Converting ChannelPreview and ChannelPreviewMessenger to hooks

@vishalnarkhede vishalnarkhede force-pushed the vishal/channel-preview-messenger-tests branch from 71b93de to eab5de0 Compare July 28, 2020 12:31
@vishalnarkhede vishalnarkhede marked this pull request as ready for review July 28, 2020 12:33
@vishalnarkhede vishalnarkhede changed the title Vishal/channel preview messenger tests CRNS-116: Vishal/channel preview messenger tests Jul 28, 2020
@vishalnarkhede vishalnarkhede force-pushed the vishal/channel-preview-messenger-tests branch from eab5de0 to 9001c9d Compare July 28, 2020 12:40
@vishalnarkhede vishalnarkhede force-pushed the vishal/channel-preview-messenger-tests branch from 9001c9d to 8f80d79 Compare July 28, 2020 13:08
@vishalnarkhede vishalnarkhede changed the title CRNS-116: Vishal/channel preview messenger tests CRNS-116: Channel preview tests and hooks Jul 28, 2020
Copy link
Contributor

@virdesai virdesai left a comment

Choose a reason for hiding this comment

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

I'd like to say we should try to alphabetize things for best practice like lists where order doesn't matter, and objects based on key name or functions based on name. make it easier to search within the code if it's following some natural mental flow

"prettier": "^1.16.4",
"react": "^16.5.0",
"react-dom": "^16.8.6",
"react-native": "0.61.5",
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason why we're a couple versions behind on RN?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DanC5 Yeah, I can't seem to remember exactly but it was breaking for 0.62 and 0.63. But give it a shot!!

return () => {
channel.off('message.new', handleNewMessageEvent);
};
});
Copy link
Contributor

Choose a reason for hiding this comment

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

do you want these to setup just on mount/unmount?

Suggested change
});
}, []);

Copy link
Contributor

Choose a reason for hiding this comment

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

tested this and the listener won't fire on each new message if we only call on mount. we have an issue tho where when you send a message in a Channel and then nav back to the ChannelList, the ChannelPreview still holds the original value and not the most recent message, looking into it now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh shit ... stupid mistake on my end!!

Copy link
Contributor

Choose a reason for hiding this comment

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

@vishalnarkhede i fixed it, all good!

Copy link
Contributor

Choose a reason for hiding this comment

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

the useLatestMessagePreview useEffect needed to be triggered on lastMessage, not channel

return () => {
channel.off('message.read', handleReadEvent);
};
});
Copy link
Contributor

Choose a reason for hiding this comment

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

do you want these to setup just on mount/unmount?

Suggested change
});
}, []);


ChannelPreview.propTypes = {
channel: PropTypes.object.isRequired,
client: PropTypes.object.isRequired,
Copy link
Contributor

Choose a reason for hiding this comment

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

this is no longer a required prop as we're pulling it from ChatContext, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

true, and we're going to axe all these prop types anyways shortly with TS

Dan Carbonell and others added 12 commits July 28, 2020 09:44
…me.js

Co-authored-by: Vir Desai <virdesai@users.noreply.github.com>
…me.js

Co-authored-by: Vir Desai <virdesai@users.noreply.github.com>
…atar.js

Co-authored-by: Vir Desai <virdesai@users.noreply.github.com>
Co-authored-by: Vir Desai <virdesai@users.noreply.github.com>
Co-authored-by: Vir Desai <virdesai@users.noreply.github.com>
Co-authored-by: Vir Desai <virdesai@users.noreply.github.com>
Co-authored-by: Vir Desai <virdesai@users.noreply.github.com>
Co-authored-by: Vir Desai <virdesai@users.noreply.github.com>
…r.test.js

Co-authored-by: Vir Desai <virdesai@users.noreply.github.com>
…r.test.js

Co-authored-by: Vir Desai <virdesai@users.noreply.github.com>
Co-authored-by: Vir Desai <virdesai@users.noreply.github.com>
DanC5 and others added 13 commits July 28, 2020 10:11
Co-authored-by: Vir Desai <virdesai@users.noreply.github.com>
Co-authored-by: Vir Desai <virdesai@users.noreply.github.com>
Co-authored-by: Vir Desai <virdesai@users.noreply.github.com>
Co-authored-by: Vir Desai <virdesai@users.noreply.github.com>
Co-authored-by: Vir Desai <virdesai@users.noreply.github.com>
Co-authored-by: Vir Desai <virdesai@users.noreply.github.com>
Co-authored-by: Vir Desai <virdesai@users.noreply.github.com>
Co-authored-by: Vir Desai <virdesai@users.noreply.github.com>
…etStream/stream-chat-react-native into vishal/channel-preview-messenger-tests
@vishalnarkhede vishalnarkhede merged commit 4eec2fb into vishal/RN-2.0 Jul 28, 2020
@delete-merged-branch delete-merged-branch bot deleted the vishal/channel-preview-messenger-tests branch July 28, 2020 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants